From 40cf00f46580bbf724d2e3d5f26d5a5c44c2532f Mon Sep 17 00:00:00 2001 From: Adam Stone Date: Fri, 23 Feb 2018 22:54:46 -0800 Subject: [PATCH] code cleanup and pin utils tests --- src/cli/commands/pin/add.js | 2 +- src/cli/commands/pin/ls.js | 2 +- src/cli/commands/pin/rm.js | 2 +- src/core/components/dag.js | 5 +- src/core/components/pin-set.js | 40 +++----- src/core/components/pin.js | 165 +++++++++++++++------------------ src/core/utils.js | 58 ++++++------ test/cli/commands.js | 2 +- test/core/interface/pin.js | 28 ++++-- test/core/utils.spec.js | 133 ++++++++++++++++++++++++++ 10 files changed, 281 insertions(+), 156 deletions(-) create mode 100644 test/core/utils.spec.js diff --git a/src/cli/commands/pin/add.js b/src/cli/commands/pin/add.js index ac7b90ae92..1c6ab15bcc 100644 --- a/src/cli/commands/pin/add.js +++ b/src/cli/commands/pin/add.js @@ -18,7 +18,7 @@ module.exports = { const paths = argv['ipfs-path'].split(' ') const recursive = argv.recursive const type = recursive ? 'recursive' : 'direct' - argv.ipfs.pin.add(paths[0], { recursive }, (err, results) => { + argv.ipfs.pin.add(paths[0], { recursive: recursive }, (err, results) => { if (err) { throw err } results.forEach((res) => { console.log(`pinned ${res.hash} ${type}ly`) diff --git a/src/cli/commands/pin/ls.js b/src/cli/commands/pin/ls.js index 9a855d56b8..0702eba1b5 100644 --- a/src/cli/commands/pin/ls.js +++ b/src/cli/commands/pin/ls.js @@ -29,7 +29,7 @@ module.exports = { const paths = argv.path && argv.path.split(' ') const type = argv.type const quiet = argv.quiet - argv.ipfs.pin.ls(paths, { type }, (err, results) => { + argv.ipfs.pin.ls(paths, { type: type }, (err, results) => { if (err) { throw err } results.forEach((res) => { let line = res.hash diff --git a/src/cli/commands/pin/rm.js b/src/cli/commands/pin/rm.js index 1eebe15db6..bef493aae3 100644 --- a/src/cli/commands/pin/rm.js +++ b/src/cli/commands/pin/rm.js @@ -17,7 +17,7 @@ module.exports = { handler: (argv) => { const paths = argv['ipfs-path'].split(' ') const recursive = argv.recursive - argv.ipfs.pin.rm(paths, { recursive }, (err, results) => { + argv.ipfs.pin.rm(paths, { recursive: recursive }, (err, results) => { if (err) { throw err } results.forEach((res) => { console.log(`unpinned ${res.hash}`) diff --git a/src/core/components/dag.js b/src/core/components/dag.js index 4ee6612f1a..769f024c37 100644 --- a/src/core/components/dag.js +++ b/src/core/components/dag.js @@ -77,7 +77,8 @@ module.exports = function dag (self) { ) }), - getRecursive: promisify((multihash, callback) => { + // TODO - move to IPLD resolver and generalize to other IPLD formats + _getRecursive: promisify((multihash, callback) => { // gets flat array of all DAGNodes in tree given by multihash callback = once(callback) self.dag.get(new CID(multihash), (err, res) => { @@ -90,7 +91,7 @@ module.exports = function dag (self) { } // branch case links.forEach(link => { - self.dag.getRecursive(link.multihash, (err, subNodes) => { + self.dag._getRecursive(link.multihash, (err, subNodes) => { if (err) { return callback(err) } nodes.push(subNodes) if (nodes.length === links.length + 1) { diff --git a/src/core/components/pin-set.js b/src/core/components/pin-set.js index 1bb3138b60..4a3319dc94 100644 --- a/src/core/components/pin-set.js +++ b/src/core/components/pin-set.js @@ -65,7 +65,6 @@ exports = module.exports = function (dag) { hasChild: (root, childhash, callback, _links, _checked, _seen) => { // callback (err, has) callback = once(callback) - if (callback.called) { return } if (typeof childhash === 'object') { childhash = toB58String(childhash) } @@ -83,9 +82,7 @@ exports = module.exports = function (dag) { return callback(null, true) } dag.get(new CID(link.multihash), (err, res) => { - if (err) { - return callback(err) - } + if (err) { return callback(err) } // don't check the same links twice if (bs58link in _seen) { return } _seen[bs58link] = true @@ -180,15 +177,9 @@ exports = module.exports = function (dag) { hashed[h].push(items[i]) } const storeItemsCb = (err, child) => { - if (callback.called) { return } - if (err) { - return callback(err) - } + if (err) { return callback(err) } dag.put(child, (err) => { - if (callback.called) { return } - if (err) { - return callback(err) - } + if (err) { return callback(err) } logInternalKey(child.multihash) rootLinks[this.h] = new DAGLink( '', child.size, child.multihash @@ -203,19 +194,18 @@ exports = module.exports = function (dag) { } }) } - _subcalls += Object.keys(hashed).length - for (let h in hashed) { - if (hashed.hasOwnProperty(h)) { - pinSet.storeItems( - hashed[h], - logInternalKey, - storeItemsCb.bind({h: h}), - _depth + 1, - _subcalls, - _done - ) - } - } + const hashedKeys = Object.keys(hashed) + _subcalls += hashedKeys.length + hashedKeys.forEach(h => { + pinSet.storeItems( + hashed[h], + logInternalKey, + storeItemsCb.bind({h: h}), + _depth + 1, + _subcalls, + _done + ) + }) } }, diff --git a/src/core/components/pin.js b/src/core/components/pin.js index 37a6b067e6..7bb8b2173f 100644 --- a/src/core/components/pin.js +++ b/src/core/components/pin.js @@ -10,6 +10,7 @@ const promisify = require('promisify-es6') const multihashes = require('multihashes') const Key = require('interface-datastore').Key const each = require('async/each') +const series = require('async/series') const waterfall = require('async/waterfall') const until = require('async/until') const once = require('once') @@ -44,7 +45,6 @@ module.exports = function pin (self) { set: pinSet(dag), add: promisify((hashes, options, callback) => { - // callback (err, pinset) if (typeof options === 'function') { callback = options options = null @@ -53,65 +53,61 @@ module.exports = function pin (self) { const recursive = !options || options.recursive !== false normalizeHashes(self, hashes, (err, mhs) => { if (err) { return callback(err) } - const result = { - // async result queue - payload: [], - update: (hash) => { - result.payload.push({hash}) - if (result.payload.length === mhs.length) { - pin.flush((err, root) => { - if (err) { return callback(err) } - return callback(null, result.payload) - }) - } - } - } - mhs.forEach((multihash) => { + // verify that each hash can be pinned + series(mhs.map(multihash => cb => { const key = toB58String(multihash) if (recursive) { if (recursivePins.has(key)) { // it's already pinned recursively - result.update(key) - return + return cb(null, key) } - - // recursive pin should replace direct pin - directPins.delete(key) - - // entire graph of nested links should be - // pinned, so make sure we have all the objects - dag.getRecursive(multihash, (err) => { - if (err) { return callback(err) } + // entire graph of nested links should be pinned, + // so make sure we have all the objects + dag._getRecursive(multihash, (err) => { + if (err) { return cb(err) } // found all objects, we can add the pin - recursivePins.add(key) - result.update(key) + return cb(null, key) }) } else { if (recursivePins.has(key)) { // recursive supersedes direct, can't have both - return callback( + return cb( new Error(`${key} already pinned recursively`) ) } if (directPins.has(key)) { // already directly pinned - result.update(key) - return + return cb(null, key) } // make sure we have the object - dag.get(new CID(multihash), (err, res) => { - if (err) { return callback(err) } + dag.get(new CID(multihash), (err) => { + if (err) { return cb(err) } // found the object, we can add the pin - directPins.add(key) - result.update(key) + return cb(null, key) + }) + } + }), (err, results) => { + if (err) { return callback(err) } + // update the pin sets in memory + if (recursive) { + results.forEach(key => { + // recursive pin should replace direct pin + directPins.delete(key) + recursivePins.add(key) }) + } else { + results.forEach(key => directPins.add(key)) } + // persist updated pin sets to datastore + pin.flush((err, root) => { + if (err) { return callback(err) } + return callback(null, results.map(key => ({hash: key}))) + }) }) }) }), rm: promisify((hashes, options, callback) => { - // callback (err) let recursive = true if (typeof options === 'function') { callback = options @@ -121,48 +117,45 @@ module.exports = function pin (self) { callback = once(callback) normalizeHashes(self, hashes, (err, mhs) => { if (err) { return callback(err) } - const result = { - // async result queue - payload: [], - update: (hash) => { - result.payload.push({hash}) - if (result.payload.length === mhs.length) { - pin.flush((err, root) => { - if (err) { return callback(err) } - return callback(null, result.payload) - }) - } - } - } - mhs.forEach((multihash) => { - pin.isPinnedWithType(multihash, pin.types.all, (err, pinned, reason) => { - if (err) { return callback(err) } - if (!pinned) { return callback(new Error('not pinned')) } + // verify that each hash can be unpinned + series(mhs.map(multihash => cb => { + pin.isPinnedWithType(multihash, pin.types.all, (err, res) => { + if (err) { return cb(err) } + const { pinned, reason } = res + if (!pinned) { return cb(new Error('not pinned')) } const key = toB58String(multihash) switch (reason) { case (pin.types.recursive): if (recursive) { - recursivePins.delete(key) - return result.update(key) + return cb(null, key) + } else { + return cb(new Error( + `${key} is pinned recursively` + )) } - return callback(new Error( - `${key} is pinned recursively` - )) case (pin.types.direct): - directPins.delete(key) - return result.update(key) + return cb(null, key) default: - return callback(new Error( + return cb(new Error( `${key} is pinned indirectly under ${reason}` )) } }) + }), (err, results) => { + if (err) { return callback(err) } + // update the pin sets in memory + const pins = recursive ? recursivePins : directPins + results.forEach(key => pins.delete(key)) + // persist updated pin sets to datastore + pin.flush((err, root) => { + if (err) { return callback(err) } + return callback(null, results.map(key => ({hash: key}))) + }) }) }) }), ls: promisify((hashes, options, callback) => { - // callback (err, pinset) let type = pin.types.all if (typeof hashes === 'function') { callback = hashes @@ -186,45 +179,40 @@ module.exports = function pin (self) { )) } if (hashes) { + // check the pinned state of specific hashes normalizeHashes(self, hashes, (err, mhs) => { if (err) { return callback(err) } - const result = { - // async result queue - payload: [], - update: (item) => { - result.payload.push(item) - if (result.payload.length === mhs.length) { - return callback(null, result.payload) - } - } - } - mhs.forEach((multihash) => { - pin.isPinnedWithType(multihash, type, (err, pinned, reason) => { - if (err) { return callback(err) } + series(mhs.map(multihash => cb => { + pin.isPinnedWithType(multihash, pin.types.all, (err, res) => { + if (err) { return cb(err) } + const { pinned, reason } = res const key = toB58String(multihash) if (!pinned) { - return callback(new Error( + return cb(new Error( `Path ${key} is not pinned` )) } switch (reason) { case pin.types.direct: case pin.types.recursive: - result.update({ + return cb(null, { hash: key, type: reason }) - break default: - result.update({ + return cb(null, { hash: key, type: `${pin.types.indirect} through ${reason}` }) } }) + }), (err, results) => { + if (err) { return callback(err) } + return callback(null, results) }) }) } else { + // show all pinned items of type const result = [] if (type === pin.types.direct || type === pin.types.all) { pin.directKeyStrings().forEach((hash) => { @@ -260,35 +248,33 @@ module.exports = function pin (self) { }), isPinned: (multihash, callback) => { - // callback (err, pinned, reason) pin.isPinnedWithType(multihash, pin.types.all, callback) }, isPinnedWithType: (multihash, pinType, callback) => { - // callback (err, pinned, reason) const key = toB58String(multihash) // recursive if ((pinType === pin.types.recursive || pinType === pin.types.all) && recursivePins.has(key)) { - return callback(null, true, pin.types.recursive) + return callback(null, {pinned: true, reason: pin.types.recursive}) } if ((pinType === pin.types.recursive)) { - return callback(null, false) + return callback(null, {pinned: false}) } // direct if ((pinType === pin.types.direct || pinType === pin.types.all) && directPins.has(key)) { - return callback(null, true, pin.types.direct) + return callback(null, {pinned: true, reason: pin.types.direct}) } if ((pinType === pin.types.direct)) { - return callback(null, false) + return callback(null, {pinned: false}) } if ((pinType === pin.types.internal || pinType === pin.types.all) && internalPins.has(key)) { - return callback(null, true, pin.types.internal) + return callback(null, {pinned: true, reason: pin.types.internal}) } if ((pinType === pin.types.internal)) { - return callback(null, false) + return callback(null, {pinned: false}) } // indirect (default) @@ -312,7 +298,7 @@ module.exports = function pin (self) { }, (err, result) => { if (err) { return callback(err) } - return callback(null, found, result) + return callback(null, {pinned: found, reason: result}) } ) }, @@ -330,14 +316,13 @@ module.exports = function pin (self) { internalKeys: () => pin.internalKeyStrings().map(key => multihashes.fromB58String(key)), getIndirectKeys: (callback) => { - // callback (err, keys) const indirectKeys = new Set() const rKeys = pin.recursiveKeys() if (!rKeys.length) { return callback(null, []) } each(rKeys, (multihash, cb) => { - dag.getRecursive(multihash, (err, nodes) => { + dag._getRecursive(multihash, (err, nodes) => { if (err) { return cb(err) } nodes.forEach((node) => { const key = toB58String(node.multihash) @@ -357,7 +342,6 @@ module.exports = function pin (self) { // encodes and writes pin key sets to the datastore // each key set will be stored as a DAG node, and a root node will link to both flush: promisify((callback) => { - // callback (err, root) const newInternalPins = new Set() const logInternalKey = (mh) => newInternalPins.add(toB58String(mh)) const handle = { @@ -396,7 +380,6 @@ module.exports = function pin (self) { }), load: promisify((callback) => { - // callback (err) const newInternalPins = new Set() const logInternalKey = (mh) => newInternalPins.add(toB58String(mh)) const handle = { diff --git a/src/core/utils.js b/src/core/utils.js index c6304fefda..e8a1f9371b 100644 --- a/src/core/utils.js +++ b/src/core/utils.js @@ -1,23 +1,34 @@ 'use strict' const multihashes = require('multihashes') -const each = require('async/each') +const mapSeries = require('async/mapSeries') +const CID = require('cids') exports.OFFLINE_ERROR = 'This command must be run in online mode. Try running \'ipfs daemon\' first.' -const splitIpfsPath = exports.splitIpfsPath = function (pathString) { +const parseIpfsPath = exports.parseIpfsPath = function (pathString) { // example: '/ipfs/b58Hash/links/by/name' // -> { root: 'b58Hash', links: ['links', 'by', 'name'] } const matched = pathString.match(/^(?:\/ipfs\/)?([^/]+(?:\/[^/]+)*)\/?$/) + const errorResult = () => ({ + error: new Error('invalid ipfs ref path') + }) if (!matched) { - return { - error: new Error('invalid ipfs ref path') - } + return errorResult() } const split = matched[1].split('/') - return { - root: split[0], - links: split.slice(1, split.length) + const root = split[0] + try { + if (CID.isCID(new CID(root))) { + return { + root: root, + links: split.slice(1, split.length) + } + } else { + return errorResult() + } + } catch (err) { + return errorResult() } } @@ -28,33 +39,26 @@ exports.normalizeHashes = function (ipfs, hashes, callback) { if (!Array.isArray(hashes)) { hashes = [hashes] } - const normalized = { - hashes: [], - update: (multihash, cb) => { + mapSeries(hashes, (hash, cb) => { + const validate = (mh) => { try { - multihashes.validate(multihash) - } catch (err) { return cb(err) } - - normalized.hashes.push(multihash) - cb() + multihashes.validate(mh) + cb(null, mh) + } catch (err) { cb(err) } } - } - each(hashes, (hash, cb) => { if (typeof hash === 'string') { - const {error, root, links} = splitIpfsPath(hash) + const {error, root, links} = parseIpfsPath(hash) const rootHash = multihashes.fromB58String(root) if (error) return cb(error) if (!links.length) { - normalized.update(rootHash, cb) + return validate(rootHash) } else { // recursively follow named links to the target const pathFn = (err, obj) => { if (err) { return cb(err) } if (links.length) { const linkName = links.shift() - const nextLink = obj.links.filter((link) => { - return (link.name === linkName) - }) + const nextLink = obj.links.filter(link => link.name === linkName) if (!nextLink.length) { return cb(new Error( `no link named ${linkName} under ${obj.toJSON().Hash}` @@ -63,16 +67,16 @@ exports.normalizeHashes = function (ipfs, hashes, callback) { const nextHash = nextLink[0].multihash ipfs.object.get(nextHash, pathFn) } else { - normalized.update(obj.multihash, cb) + validate(obj.multihash) } } ipfs.object.get(rootHash, pathFn) } } else { - normalized.update(hash, cb) + validate(hash) } - }, (err) => { + }, (err, results) => { if (err) { return callback(err) } - return callback(null, normalized.hashes) + return callback(null, results) }) } diff --git a/test/cli/commands.js b/test/cli/commands.js index f286c31c94..932a68b01f 100644 --- a/test/cli/commands.js +++ b/test/cli/commands.js @@ -4,7 +4,7 @@ const expect = require('chai').expect const runOnAndOff = require('../utils/on-and-off') -const commandCount = 75 +const commandCount = 76 describe('commands', () => runOnAndOff((thing) => { let ipfs diff --git a/test/core/interface/pin.js b/test/core/interface/pin.js index 90bfc8f2e9..e0a3ae58b2 100644 --- a/test/core/interface/pin.js +++ b/test/core/interface/pin.js @@ -2,17 +2,31 @@ 'use strict' const test = require('interface-ipfs-core') -const IPFSFactory = require('../../utils/ipfs-factory-instance') +const parallel = require('async/parallel') -let factory +const IPFS = require('../../../src') +const DaemonFactory = require('ipfsd-ctl') +const df = DaemonFactory.create({ type: 'proc', exec: IPFS }) + +const nodes = [] const common = { - setup: function (cb) { - factory = new IPFSFactory() - cb(null, factory) + setup: function (callback) { + callback(null, { + spawnNode: (cb) => { + df.spawn((err, _ipfsd) => { + if (err) { + return cb(err) + } + + nodes.push(_ipfsd) + cb(null, _ipfsd.api) + }) + } + }) }, - teardown: function (cb) { - factory.dismantle(cb) + teardown: function (callback) { + parallel(nodes.map((node) => (cb) => node.stop(cb)), callback) } } diff --git a/test/core/utils.spec.js b/test/core/utils.spec.js new file mode 100644 index 0000000000..df2141adfe --- /dev/null +++ b/test/core/utils.spec.js @@ -0,0 +1,133 @@ +/* eslint max-nested-callbacks: ["error", 8] */ +/* eslint-env mocha */ +'use strict' + +const chai = require('chai') +const dirtyChai = require('dirty-chai') +const expect = chai.expect +chai.use(dirtyChai) +const multihashes = require('multihashes') +const utils = require('../../src/core/utils') + +// This gets replaced by `create-repo-browser.js` in the browser +const createTempRepo = require('../utils/create-repo-nodejs.js') + +const IPFS = require('../../src/core') + +describe('utils', () => { + const rootHashString = 'QmUhUuiTKkkK8J6JZ9zmj8iNHPuNfGYcszgRumzhHBxEEU' + const rootHash = multihashes.fromB58String(rootHashString) + const rootPathString = `/ipfs/${rootHashString}/` + const aboutHashString = 'QmZTR5bcpQD7cFgTorqxZDYaew1Wqgfbd2ud9QqGPAkK2V' + const aboutHash = multihashes.fromB58String(aboutHashString) + const aboutPathString = `/ipfs/${rootHashString}/about` + + describe('parseIpfsPath', () => { + it('parses path with no links', function () { + expect(utils.parseIpfsPath(rootHashString)) + .to.deep.equal({ + root: rootHashString, + links: [] + }) + }) + it('parses path with links', function () { + expect(utils.parseIpfsPath(`${rootHashString}/docs/index`)) + .to.deep.equal({ + root: rootHashString, + links: ['docs', 'index'] + }) + }) + it('parses path with /ipfs/ prefix', function () { + expect(utils.parseIpfsPath(`/ipfs/${rootHashString}/about`)) + .to.deep.equal({ + root: rootHashString, + links: ['about'] + }) + }) + it('returns error for malformed path', function () { + const result = utils.parseIpfsPath(`${rootHashString}//about`) + expect(result.error).to.be.instanceof(Error) + .and.have.property('message', 'invalid ipfs ref path') + }) + it('returns error if root is not a valid CID', function () { + const result = utils.parseIpfsPath('invalid/ipfs/path') + expect(result.error).to.be.instanceof(Error) + .and.have.property('message', 'invalid ipfs ref path') + }) + }) + + describe('normalizeHashes', function () { + this.timeout(80 * 1000) + let node + let repo + + before((done) => { + repo = createTempRepo() + node = new IPFS({ + repo: repo + }) + node.once('ready', done) + }) + + after((done) => { + repo.teardown(done) + }) + + it('normalizes hash string to array with multihash object', (done) => { + utils.normalizeHashes(node, rootHashString, (err, hashes) => { + expect(err).to.not.exist() + expect(hashes.length).to.equal(1) + expect(hashes[0]).to.deep.equal(rootHash) + done() + }) + }) + + it('normalizes array of hash strings to array of multihash objects', (done) => { + utils.normalizeHashes(node, [rootHashString, aboutHashString], (err, hashes) => { + expect(err).to.not.exist() + expect(hashes.length).to.equal(2) + expect(hashes[0]).to.deep.equal(rootHash) + expect(hashes[1]).to.deep.equal(aboutHash) + done() + }) + }) + + it('normalizes multihash object to array with multihash object', (done) => { + utils.normalizeHashes(node, aboutHash, (err, hashes) => { + expect(err).to.not.exist() + expect(hashes.length).to.equal(1) + expect(hashes[0]).to.deep.equal(aboutHash) + done() + }) + }) + + it('normalizes array of multihash objects to array of multihash objects', (done) => { + utils.normalizeHashes(node, [rootHash, aboutHash], (err, hashes) => { + expect(err).to.not.exist() + expect(hashes.length).to.equal(2) + expect(hashes[0]).to.deep.equal(rootHash) + expect(hashes[1]).to.deep.equal(aboutHash) + done() + }) + }) + + it('normalizes ipfs path string to array with multihash object', (done) => { + utils.normalizeHashes(node, aboutPathString, (err, hashes) => { + expect(err).to.not.exist() + expect(hashes.length).to.equal(1) + expect(hashes[0]).to.deep.equal(aboutHash) + done() + }) + }) + + it('normalizes array of ipfs path strings to array with multihash objects', (done) => { + utils.normalizeHashes(node, [aboutPathString, rootPathString], (err, hashes) => { + expect(err).to.not.exist() + expect(hashes.length).to.equal(2) + expect(hashes[0]).to.deep.equal(aboutHash) + expect(hashes[1]).to.deep.equal(rootHash) + done() + }) + }) + }) +})