From ef824d3e7894e9f6df7ccc40e32e84c0f64e2868 Mon Sep 17 00:00:00 2001 From: Hugo Dias Date: Thu, 16 May 2019 16:28:33 +0200 Subject: [PATCH] fix: make recursive true defaults, tests and more --- package.json | 3 +- src/cli/commands/name/publish.js | 22 +- src/cli/commands/name/resolve.js | 2 +- src/core/components/name.js | 2 +- src/core/ipns/index.js | 21 +- src/core/ipns/publisher.js | 5 +- src/http/api/resources/name.js | 2 +- src/utils/tlru.js | 83 ++++++++ test/cli/name-pubsub.js | 50 ----- test/cli/name.js | 336 +++++-------------------------- test/core/name-pubsub.js | 30 +++ 11 files changed, 190 insertions(+), 366 deletions(-) create mode 100644 src/utils/tlru.js diff --git a/package.json b/package.json index c1fd6af893..7740981c33 100644 --- a/package.json +++ b/package.json @@ -65,6 +65,7 @@ "aegir": "^18.1.0", "base64url": "^3.0.1", "chai": "^4.2.0", + "clear-module": "^3.2.0", "delay": "^4.1.0", "detect-node": "^2.0.4", "dir-compare": "^1.4.0", @@ -105,6 +106,7 @@ "get-folder-size": "^2.0.0", "glob": "^7.1.3", "hapi-pino": "^5.4.1", + "hashlru": "^2.3.0", "human-to-milliseconds": "^1.0.0", "interface-datastore": "~0.6.0", "ipfs-bitswap": "~0.23.0", @@ -175,7 +177,6 @@ "pull-stream": "^3.6.9", "pull-stream-to-stream": "^1.3.4", "readable-stream": "^3.1.1", - "receptacle": "^1.3.2", "stream-to-pull-stream": "^1.7.3", "superstruct": "~0.6.0", "tar-stream": "^2.0.0", diff --git a/src/cli/commands/name/publish.js b/src/cli/commands/name/publish.js index bb67db22d0..57d2f733ab 100644 --- a/src/cli/commands/name/publish.js +++ b/src/cli/commands/name/publish.js @@ -1,6 +1,6 @@ 'use strict' -const print = require('../../utils').print +const { print } = require('../../utils') module.exports = { command: 'publish ', @@ -11,21 +11,25 @@ module.exports = { resolve: { alias: 'r', describe: 'Resolve given path before publishing. Default: true.', - default: true + default: true, + type: 'boolean' }, lifetime: { alias: 't', describe: 'Time duration that the record will be valid for. Default: 24h.', - default: '24h' + default: '24h', + type: 'string' }, key: { alias: 'k', describe: 'Name of the key to be used or a valid PeerID, as listed by "ipfs key list -l". Default: self.', - default: 'self' + default: 'self', + type: 'string' }, ttl: { describe: 'Time duration this record should be cached for (caution: experimental).', - default: '' + default: '', + type: 'string' } }, @@ -33,14 +37,8 @@ module.exports = { argv.resolve((async () => { // yargs-promise adds resolve/reject properties to argv // resolve should use the alias as resolve will always be overwritten to a function - let resolve = true - - if (argv.r === false || argv.r === 'false') { - resolve = false - } - const opts = { - resolve, + resolve: argv.r, lifetime: argv.lifetime, key: argv.key, ttl: argv.ttl diff --git a/src/cli/commands/name/resolve.js b/src/cli/commands/name/resolve.js index 8c83b25682..ca1dd219a3 100644 --- a/src/cli/commands/name/resolve.js +++ b/src/cli/commands/name/resolve.js @@ -18,7 +18,7 @@ module.exports = { type: 'boolean', alias: 'r', describe: 'Resolve until the result is not an IPNS name. Default: false.', - default: false + default: true } }, diff --git a/src/core/components/name.js b/src/core/components/name.js index be281964a3..99cd4f5fc4 100644 --- a/src/core/components/name.js +++ b/src/core/components/name.js @@ -152,7 +152,7 @@ module.exports = function name (self) { options = mergeOptions({ nocache: false, - recursive: false + recursive: true }, options) const offline = self._options.offline diff --git a/src/core/ipns/index.js b/src/core/ipns/index.js index e693e21f0f..d7405ca3e7 100644 --- a/src/core/ipns/index.js +++ b/src/core/ipns/index.js @@ -2,7 +2,6 @@ const { createFromPrivKey } = require('peer-id') const series = require('async/series') -const Receptacle = require('receptacle') const errcode = require('err-code') const debug = require('debug') @@ -13,7 +12,8 @@ const IpnsPublisher = require('./publisher') const IpnsRepublisher = require('./republisher') const IpnsResolver = require('./resolver') const path = require('./path') - +const { normalizePath } = require('../utils') +const TLRU = require('../../utils/tlru') const defaultRecordTtl = 60 * 1000 class IPNS { @@ -21,12 +21,19 @@ class IPNS { this.publisher = new IpnsPublisher(routing, datastore) this.republisher = new IpnsRepublisher(this.publisher, datastore, peerInfo, keychain, options) this.resolver = new IpnsResolver(routing) - this.cache = new Receptacle({ max: 1000 }) // Create an LRU cache with max 1000 items + this.cache = new TLRU(1000) this.routing = routing } // Publish - publish (privKey, value, lifetime, callback) { + publish (privKey, value, lifetime = IpnsPublisher.defaultRecordLifetime, callback) { + try { + value = normalizePath(value) + } catch (err) { + log.error(err) + return callback(err) + } + series([ (cb) => createFromPrivKey(privKey.bytes, cb), (cb) => this.publisher.publishWithEOL(privKey, value, lifetime, cb) @@ -38,12 +45,12 @@ class IPNS { log(`IPNS value ${value} was published correctly`) - // Add to cache + // // Add to cache const id = results[0].toB58String() const ttEol = parseFloat(lifetime) const ttl = (ttEol < defaultRecordTtl) ? ttEol : defaultRecordTtl - this.cache.set(id, value, { ttl: ttl }) + this.cache.set(id, value, ttl) log(`IPNS value ${value} was cached correctly`) @@ -96,7 +103,7 @@ class IPNS { // Initialize keyspace // sets the ipns record for the given key to point to an empty directory initializeKeyspace (privKey, value, callback) { - this.publisher.publish(privKey, value, callback) + this.publish(privKey, value, IpnsPublisher.defaultRecordLifetime, callback) } } diff --git a/src/core/ipns/publisher.js b/src/core/ipns/publisher.js index 8caa9c8dba..18027632f6 100644 --- a/src/core/ipns/publisher.js +++ b/src/core/ipns/publisher.js @@ -11,7 +11,7 @@ log.error = debug('ipfs:ipns:publisher:error') const ipns = require('ipns') -const defaultRecordTtl = 60 * 60 * 1000 +const defaultRecordLifetime = 60 * 60 * 1000 // IpnsPublisher is capable of publishing and resolving names to the IPFS routing system. class IpnsPublisher { @@ -46,7 +46,7 @@ class IpnsPublisher { // Accepts a keypair, as well as a value (ipfsPath), and publishes it out to the routing system publish (privKey, value, callback) { - this.publishWithEOL(privKey, value, defaultRecordTtl, callback) + this.publishWithEOL(privKey, value, defaultRecordLifetime, callback) } _putRecordToRouting (record, peerId, callback) { @@ -269,4 +269,5 @@ class IpnsPublisher { } } +IpnsPublisher.defaultRecordLifetime = defaultRecordLifetime exports = module.exports = IpnsPublisher diff --git a/src/http/api/resources/name.js b/src/http/api/resources/name.js index f42c770508..02b1eb7326 100644 --- a/src/http/api/resources/name.js +++ b/src/http/api/resources/name.js @@ -7,7 +7,7 @@ exports.resolve = { query: Joi.object().keys({ arg: Joi.string(), nocache: Joi.boolean().default(false), - recursive: Joi.boolean().default(false) + recursive: Joi.boolean().default(true) }).unknown() }, async handler (request, h) { diff --git a/src/utils/tlru.js b/src/utils/tlru.js new file mode 100644 index 0000000000..c60cc35e55 --- /dev/null +++ b/src/utils/tlru.js @@ -0,0 +1,83 @@ +'use strict' +const hashlru = require('hashlru') + +/** + * Time Aware Least Recent Used Cache + * @see https://arxiv.org/pdf/1801.00390 + * @todo move this to ipfs-utils or it's own package + * + * @class TLRU + */ +class TLRU { + /** + * Creates an instance of TLRU. + * + * @param {number} maxSize + * @memberof TLRU + */ + constructor (maxSize) { + this.lru = hashlru(maxSize) + } + + /** + * Get the value from the a key + * + * @param {string} key + * @returns {any} + * @memberof TLRU + */ + get (key) { + const value = this.lru.get(key) + if (value) { + if ((value.expire) && (value.expire < Date.now())) { + this.lru.remove(key) + return undefined + } + } + return value.value + } + + /** + * Set a key value pair + * + * @param {string} key + * @param {any} value + * @param {number} ttl - in miliseconds + * @memberof TLRU + */ + set (key, value, ttl) { + this.lru.set(key, { value, expire: Date.now() + ttl }) + } + + /** + * Find if the cache has the key + * + * @param {string} key + * @returns {boolean} + * @memberof TLRU + */ + has (key) { + return this.lru.has(key) + } + + /** + * Remove key + * + * @param {string} key + * @memberof TLRU + */ + remove (key) { + this.lru.remove(key) + } + + /** + * Clears the cache + * + * @memberof TLRU + */ + clear () { + this.lru.clear() + } +} + +module.exports = TLRU diff --git a/test/cli/name-pubsub.js b/test/cli/name-pubsub.js index fd125948a5..b481c3ee4f 100644 --- a/test/cli/name-pubsub.js +++ b/test/cli/name-pubsub.js @@ -14,9 +14,6 @@ const ipfsExec = require('../utils/ipfs-exec') const DaemonFactory = require('ipfsd-ctl') const df = DaemonFactory.create({ type: 'js' }) -const checkAll = (bits) => string => bits.every(bit => string.includes(bit)) -const emptyDirCid = 'QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn' - const spawnDaemon = (callback) => { df.spawn({ exec: `./src/cli/bin.js`, @@ -165,53 +162,6 @@ describe('name-pubsub', () => { }) }) }) - - describe('pubsub records', () => { - let cidAdded - - before(function (done) { - this.timeout(50 * 1000) - ipfsA('add src/init-files/init-docs/readme') - .then((out) => { - cidAdded = out.split(' ')[1] - done() - }) - }) - - it('should publish the received record to the subscriber', function () { - this.timeout(80 * 1000) - - return ipfsB(`name resolve ${nodeBId.id}`) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([emptyDirCid])) // Empty dir received (subscribed) - - return ipfsA(`name resolve ${nodeBId.id}`) - }) - .catch((err) => { - expect(err).to.exist() // Not available (subscribed now) - - return ipfsB(`name publish ${cidAdded}`) - }) - .then((res) => { - // published to IpfsB and published through pubsub to ipfsa - expect(res).to.exist() - expect(res).to.satisfy(checkAll([cidAdded, nodeBId.id])) - - return ipfsB(`name resolve ${nodeBId.id}`) - }) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([cidAdded])) - - return ipfsA(`name resolve ${nodeBId.id}`) - }) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([cidAdded])) // value propagated to node B - }) - }) - }) }) describe('disabled', () => { diff --git a/test/cli/name.js b/test/cli/name.js index c34ae70ef4..984d777da8 100644 --- a/test/cli/name.js +++ b/test/cli/name.js @@ -1,305 +1,59 @@ -/* eslint max-nested-callbacks: ["error", 6] */ /* eslint-env mocha */ 'use strict' -const chai = require('chai') -const dirtyChai = require('dirty-chai') -const expect = chai.expect -chai.use(dirtyChai) - -const hat = require('hat') -const ipfsExec = require('../utils/ipfs-exec') - -const DaemonFactory = require('ipfsd-ctl') -const df = DaemonFactory.create({ type: 'js' }) - -const checkAll = (bits) => string => bits.every(bit => string.includes(bit)) +const sinon = require('sinon') +const YargsPromise = require('yargs-promise') +const clearModule = require('clear-module') describe('name', () => { - describe('working locally', () => { - const passPhrase = hat() - const pass = '--pass ' + passPhrase - const name = 'test-key-' + hat() - - let ipfs - let ipfsd - - let cidAdded - let nodeId - let keyId - - before(function (done) { - this.timeout(80 * 1000) - - df.spawn({ - exec: `./src/cli/bin.js`, - config: { - Bootstrap: [] - }, - args: ['--pass', passPhrase, '--offline'], - initOptions: { bits: 512 } - }, (err, _ipfsd) => { - expect(err).to.not.exist() - - ipfsd = _ipfsd - ipfs = ipfsExec(_ipfsd.repoPath) - - ipfs(`${pass} key gen ${name} --type rsa --size 2048`) - .then((out) => { - expect(out).to.include(name) - keyId = out.split(' ')[1] - - return ipfs('id') - }) - .then((res) => { - const id = JSON.parse(res) - expect(id).to.have.property('id') - nodeId = id.id - - return ipfs('add src/init-files/init-docs/readme') - }) - .then((out) => { - cidAdded = out.split(' ')[1] - done() - }) - }) - }) - - after(function (done) { - if (ipfsd) { - ipfsd.stop(() => done()) - } else { - done() - } - }) - - it('should publish correctly when the file was already added', function () { - this.timeout(70 * 1000) - - return ipfs(`name publish ${cidAdded}`).then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([cidAdded, nodeId])) - }) - }) - - it('should publish and resolve an entry with the default options', function () { - this.timeout(70 * 1000) - - return ipfs(`name publish ${cidAdded}`) - .then((res) => { - expect(res).to.exist() - - return ipfs('name resolve') - }) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([cidAdded])) - }) - }) - - it('should publish correctly when the file was not added but resolve is disabled', function () { - this.timeout(70 * 1000) - - const notAddedCid = 'QmPFVLPmp9zv5Z5KUqLhe2EivAGccQW2r7M7jhVJGLZoZU' - - return ipfs(`name publish ${notAddedCid} --resolve false`).then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([notAddedCid, nodeId])) - }) - }) - - it('should not get the entry correctly if its validity time expired', function () { - this.timeout(70 * 1000) - - return ipfs(`name publish ${cidAdded} --lifetime 10ns`) - .then((res) => { - expect(res).to.exist() - - setTimeout(function () { - return ipfs('name resolve') - .then((res) => { - expect(res).to.not.exist() - }) - .catch((err) => { - expect(err).to.exist() - }) - }, 1) - }) - }) - - it('should publish correctly when a new key is used', function () { - this.timeout(70 * 1000) - - return ipfs(`name publish ${cidAdded} --key ${name}`).then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([cidAdded, keyId])) - }) - }) - - it('should return the immediate pointing record, unless using the recursive parameter', function () { - this.timeout(90 * 1000) - - return ipfs(`name publish ${cidAdded}`) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([cidAdded, nodeId])) - - return ipfs(`name publish /ipns/${nodeId} --key ${name}`) - }) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([nodeId, keyId])) - - return ipfs(`name resolve ${keyId}`) - }) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([nodeId])) - }) - }) - - it('should go recursively until finding an ipfs hash', function () { - this.timeout(90 * 1000) - - return ipfs(`name publish ${cidAdded}`) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([cidAdded, nodeId])) - - return ipfs(`name publish /ipns/${nodeId} --key ${name}`) - }) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([nodeId, keyId])) - - return ipfs(`name resolve ${keyId} --recursive`) - }) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([cidAdded])) - }) - }) + let cli + let cliUtils + beforeEach(() => { + cliUtils = require('../../src/cli/utils') + cli = new YargsPromise(require('../../src/cli/parser')) + }) + afterEach(() => { + sinon.restore() + // TODO: the lines below shouldn't be necessary, cli needs refactor to simplify testability + // Force the next require to not use require cache + clearModule('../../src/cli/utils') + clearModule('../../src/cli/parser') }) - describe('using dht', () => { - const passPhrase = hat() - const pass = '--pass ' + passPhrase - const name = 'test-key-' + hat() - - let ipfs - let ipfsd - - let cidAdded - let nodeId - let keyId - - before(function (done) { - this.timeout(80 * 1000) - - df.spawn({ - exec: `./src/cli/bin.js`, - config: { - Bootstrap: [], - Discovery: { - MDNS: { - Enabled: false - }, - webRTCStar: { - Enabled: false - } - } - }, - args: ['--pass', passPhrase], - initOptions: { bits: 512 } - }, (err, _ipfsd) => { - expect(err).to.not.exist() - - ipfsd = _ipfsd - ipfs = ipfsExec(_ipfsd.repoPath) - - ipfs(`${pass} key gen ${name} --type rsa --size 2048`) - .then((out) => { - expect(out).to.include(name) - keyId = out.split(' ')[1] - - return ipfs('id') - }) - .then((res) => { - const id = JSON.parse(res) - expect(id).to.have.property('id') - nodeId = id.id - - return ipfs('add src/init-files/init-docs/readme') - }) - .then((out) => { - cidAdded = out.split(' ')[1] - done() - }) - }) - }) - - after(function (done) { - if (ipfsd) { - ipfsd.stop(() => done()) - } else { - done() - } - }) - - it('should publish and resolve an entry with the default options', function () { - this.timeout(70 * 1000) - - return ipfs(`name publish ${cidAdded}`) - .then((res) => { - expect(res).to.exist() - - return ipfs('name resolve') - }) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([cidAdded])) - }) - }) - - it('should not get the entry correctly if its validity time expired', function () { - this.timeout(70 * 1000) - - return ipfs(`name publish ${cidAdded} --lifetime 10ns`) - .then((res) => { - expect(res).to.exist() - - setTimeout(function () { - return ipfs('name resolve') - .then((res) => { - expect(res).to.not.exist() - }) - .catch((err) => { - expect(err).to.exist() - }) - }, 1) - }) - }) + it('resolve', async () => { + const resolveFake = sinon.fake() - it('should return the immediate pointing record, unless using the recursive parameter', function () { - this.timeout(90 * 1000) + sinon + .stub(cliUtils, 'getIPFS') + .callsArgWith(1, null, { name: { resolve: resolveFake } }) - return ipfs(`name publish ${cidAdded}`) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([cidAdded, nodeId])) + // TODO: the lines below shouldn't be necessary, cli needs refactor to simplify testability + // Force the next require to not use require cache + clearModule('../../src/cli/commands/name/resolve.js') - return ipfs(`name publish /ipns/${nodeId} --key ${name}`) - }) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([nodeId, keyId])) + await cli.parse(`name resolve test`) + sinon.assert.calledWith(resolveFake, 'test', { nocache: false, recursive: true }) + }) - return ipfs(`name resolve ${keyId}`) - }) - .then((res) => { - expect(res).to.exist() - expect(res).to.satisfy(checkAll([nodeId])) - }) + it('publish', async () => { + const publishFake = sinon.fake.returns({ name: 'name', value: 'value' }) + const printSpy = sinon.spy(cliUtils, 'print') + + sinon + .stub(cliUtils, 'getIPFS') + .callsArgWith(1, null, { name: { publish: publishFake } }) + + // TODO: the lines below shouldn't be necessary, cli needs refactor to simplify testability + // Force the next require to not use require cache + clearModule('../../src/cli/commands/name/publish.js') + + await cli.parse(`name publish test --silent`) + sinon.assert.calledWith(printSpy, 'Published to name: value') + sinon.assert.calledWith(publishFake, 'test', { + resolve: true, + lifetime: '24h', + key: 'self', + ttl: '' }) }) }) diff --git a/test/core/name-pubsub.js b/test/core/name-pubsub.js index 3edb00d460..884d0a55b7 100644 --- a/test/core/name-pubsub.js +++ b/test/core/name-pubsub.js @@ -18,6 +18,7 @@ const isNode = require('detect-node') const ipns = require('ipns') const IPFS = require('../../src') const waitFor = require('../utils/wait-for') +const delay = require('interface-ipfs-core/src/utils/delay') const DaemonFactory = require('ipfsd-ctl') const df = DaemonFactory.create({ type: 'proc' }) @@ -34,6 +35,7 @@ describe('name-pubsub', function () { let nodeA let nodeB let idA + let idB const createNode = (callback) => { df.spawn({ @@ -73,6 +75,7 @@ describe('name-pubsub', function () { expect(err).to.not.exist() idA = ids[0] + idB = ids[1] nodeA.swarm.connect(ids[1].addresses[0], done) }) }) @@ -135,4 +138,31 @@ describe('name-pubsub', function () { }) }) }) + + it('should self resolve, publish and then resolve correctly', async function () { + this.timeout(6000) + const emptyDirCid = '/ipfs/QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn' + const [{ path }] = await nodeA.add(Buffer.from('pubsub records')) + + const resolvesEmpty = await nodeB.name.resolve(idB.id) + expect(resolvesEmpty).to.be.eq(emptyDirCid) + + try { + await nodeA.name.resolve(idB.id) + } catch (error) { + expect(error).to.exist() + } + + const publish = await nodeB.name.publish(path) + expect(publish).to.be.eql({ + name: idB.id, + value: `/ipfs/${path}` + }) + + const resolveB = await nodeB.name.resolve(idB.id) + expect(resolveB).to.be.eq(`/ipfs/${path}`) + await delay(5000) + const resolveA = await nodeA.name.resolve(idB.id) + expect(resolveA).to.be.eq(`/ipfs/${path}`) + }) })