Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Refactor ipld to use pull streams for get #122

Closed
wants to merge 10 commits into from
Closed

Refactor ipld to use pull streams for get #122

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
58df08f
Refactor ipld to use pull streams for get
Apr 6, 2018
94d51be
Refactor conditional logic
Apr 25, 2018
d570ab9
Prefix unused variable with _
Apr 25, 2018
c5c05cd
Explicitly check the path of results
Apr 27, 2018
1dce3d5
Add test for only node ipld get option
Apr 27, 2018
54beacb
Add explicit test for resolver.getPullStream
Apr 27, 2018
21ccc0f
Fix new tests to check for right value
Apr 27, 2018
a76efbd
Make naming consistent
May 6, 2018
ed6cd41
Fix comment to be on the correct line
May 6, 2018
2d53565
Merge branch 'master' into master
zcstarr May 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 55 additions & 49 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
const Block = require('ipfs-block')
const pull = require('pull-stream')
const CID = require('cids')
const doUntil = require('async/doUntil')
const IPFSRepo = require('ipfs-repo')
const BlockService = require('ipfs-block-service')
const joinPath = require('path').join
Expand Down Expand Up @@ -125,77 +124,84 @@ class IPLDResolver {
options = {}
}

if (!options) {
options = {}
}

pull(this.getPullStream(cid, path, options),
pull.reduce((arr, item) => {
if (!options.onlyNode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a general comment, nothing you need to change: Often code is easier to understand without negations. I would flip this if statement to:

if(options.onlyNode) {
  // replace existing entry
  arr[0] = item
} else {
  arr.push(item)
}

I might also change the comment to something like "the only item is the last one" or "reducing to the last item".

arr.push(item)
} else {
// replace existing entry
arr[0] = item
}
return arr
}, [], callback)
)
}

getPullStream (cid, path, options) {
// this removes occurrences of ./, //, ../
// makes sure that path never starts with ./ or /
// path.join is OS specific. Need to convert back to POSIX format.

if (typeof path === 'string') {
path = joinPath('/', path)
.substr(1)
.split(osPathSep)
.join('/')
}

let stop = false
if (path === '' || !path) {
return this._get(cid, (err, node) => {
if (err) {
return callback(err)
}
callback(null, {
value: node,
remainderPath: ''
return function read (abort, cb) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to prefix variables that aren't used with an underscore. This way you don't wonder if abort wasn't used accidentally or intentionally. Though I'm not sure if that's done that way in this codebase yet.

if (stop) return cb(stop)
this._get(cid, (err, node) => {
if (err) {
return cb(err)
}
stop = true
cb(null, {
value: node,
remainderPath: ''
})
})
})
}.bind(this)
}

let value

doUntil(
(cb) => {
// get block
// use local resolver
// update path value
this.bs.get(cid, (err, block) => {
return function read (abort, cb) {
if (stop) return cb(stop)
this.bs.get(cid, (err, block) => {
if (err || abort) {
return cb(err)
}
const r = this.resolvers[cid.codec]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nice trick I learnt a few years ago from a colleague. Single letter variable names are really hard to grep for, but are useful. To keep the terseness but make it easier to grep, just double the single. So this becomes const rr = this.resolvers[cid.codec].

if (!r) {
return cb(new Error('No resolver found for codec "' + cid.codec + '"'))
}
r.resolver.resolve(block.data, path, (err, result) => {
if (err) {
return cb(err)
}
const r = this.resolvers[cid.codec]
if (!r) {
return cb(new Error('No resolver found for codec "' + cid.codec + '"'))
const value = result.value
path = result.remainderPath
const endReached = !path || path === '' || path === '/'
const isTerminal = value && !value['/']

if ((endReached && isTerminal) || options.localResolve) {
stop = true
return cb(null, { value, remainderPath: path })
}
r.resolver.resolve(block.data, path, (err, result) => {
if (err) {
return cb(err)
}
value = result.value
path = result.remainderPath
cb()
})
})
},
() => {
const endReached = !path || path === '' || path === '/'
const isTerminal = value && !value['/']

if ((endReached && isTerminal) || options.localResolve) {
return true
} else {
// continue traversing
if (value) {
cid = new CID(value['/'])
}
return false
}
},
(err, results) => {
if (err) {
return callback(err)
}
return callback(null, {
value: value,
remainderPath: path

cb(null, { value, remainderPath: path })
})
}
)
})
}.bind(this)
}

getStream (cid, path, options) {
Expand Down
3 changes: 2 additions & 1 deletion test/ipld-all.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ describe('IPLD Resolver for dag-cbor + dag-pb', () => {
it('resolve through different formats', (done) => {
resolver.get(cidCbor, 'pb/Data', (err, result) => {
expect(err).to.not.exist()
expect(result.value).to.eql(Buffer.from('I am inside a Protobuf'))
expect(result.length).to.eq(2)
expect(result[1].value).to.eql(Buffer.from('I am inside a Protobuf'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be a test the also tests all results, not just the last one. Though it might make sense to make a dedicated test for it, so that the other tests are kept short to the point.

done()
})
})
Expand Down
15 changes: 9 additions & 6 deletions test/ipld-bitcoin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ module.exports = (repo) => {
expect(err).to.not.exist()
resolver.get(cid1, (err, result) => {
expect(err).to.not.exist()
expect(node1.version).to.eql(result.value.version)
expect(node1.version).to.eql(result[0].value.version)
done()
})
})
Expand All @@ -125,7 +125,7 @@ module.exports = (repo) => {
resolver.get(cid1, '/', (err, result) => {
expect(err).to.not.exist()

ipldBitcoin.util.cid(result.value, (err, cid) => {
ipldBitcoin.util.cid(result[0].value, (err, cid) => {
expect(err).to.not.exist()
expect(cid).to.eql(cid1)
done()
Expand All @@ -136,23 +136,26 @@ module.exports = (repo) => {
it('value within 1st node scope', (done) => {
resolver.get(cid1, 'version', (err, result) => {
expect(err).to.not.exist()
expect(result.value).to.eql(1)
expect(result.length).to.eq(1)
expect(result[0].value).to.eql(1)
done()
})
})

it('value within nested scope (1 level)', (done) => {
resolver.get(cid2, 'parent/version', (err, result) => {
expect(err).to.not.exist()
expect(result.value).to.eql(1)
expect(result.length).to.eq(2)
expect(result[1].value).to.eql(1)
done()
})
})

it('value within nested scope (2 levels)', (done) => {
resolver.get(cid3, 'parent/parent/version', (err, result) => {
expect(err).to.not.exist()
expect(result.value).to.eql(1)
expect(result.length).to.eq(3)
expect(result[2].value).to.eql(1)
done()
})
})
Expand All @@ -162,7 +165,7 @@ module.exports = (repo) => {
expect(err).to.not.exist()
resolver.get(cid1, (err, result) => {
expect(err).to.not.exist()
expect(result.value.version).to.eql(1)
expect(result[0].value.version).to.eql(1)
remove()
})
})
Expand Down
58 changes: 42 additions & 16 deletions test/ipld-dag-cbor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,26 @@ module.exports = (repo) => {
}, done)
})

it('resolver.getPullStream just CID', (done) => {
pull(resolver.getPullStream(cid1, null),
pull.collect((err, results) => {
expect(err).to.not.exist()
expect(results.length).to.eq(1)

dagCBOR.util.cid(results[0].value, (err, cid) => {
expect(err).to.not.exist()
expect(cid).to.eql(cid1)
})
}))
done()
})

it('resolver.get just CID', (done) => {
resolver.get(cid1, (err, result) => {
resolver.get(cid1, (err, results) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all other tests you didn't rename result to results. This is just a something I'd like to mention in case you care a lot about consistency. I'm happy to leave it like that (there's no point of wasting time on such small things, but sometimes I do then though ;)

expect(results.length).to.eq(1)
expect(err).to.not.exist()

dagCBOR.util.cid(result.value, (err, cid) => {
dagCBOR.util.cid(results[0].value, (err, cid) => {
expect(err).to.not.exist()
expect(cid).to.eql(cid1)
done()
Expand All @@ -127,10 +142,11 @@ module.exports = (repo) => {
})

it('resolver.get root path', (done) => {
resolver.get(cid1, '/', (err, result) => {
resolver.get(cid1, '/', (err, results) => {
expect(results.length).to.eq(1)
expect(err).to.not.exist()

dagCBOR.util.cid(result.value, (err, cid) => {
dagCBOR.util.cid(results[0].value, (err, cid) => {
expect(err).to.not.exist()
expect(cid).to.eql(cid1)
done()
Expand All @@ -141,8 +157,9 @@ module.exports = (repo) => {
it('resolver.get relative path `.` (same as get /)', (done) => {
resolver.get(cid1, '.', (err, result) => {
expect(err).to.not.exist()
expect(result.length).to.eq(1)

dagCBOR.util.cid(result.value, (err, cid) => {
dagCBOR.util.cid(result[0].value, (err, cid) => {
expect(err).to.not.exist()
expect(cid).to.eql(cid1)
done()
Expand All @@ -153,8 +170,9 @@ module.exports = (repo) => {
it('resolver.get relative path `./` (same as get /)', (done) => {
resolver.get(cid1, './', (err, result) => {
expect(err).to.not.exist()
expect(result.length).to.eq(1)

dagCBOR.util.cid(result.value, (err, cid) => {
dagCBOR.util.cid(result[0].value, (err, cid) => {
expect(err).to.not.exist()
expect(cid).to.eql(cid1)
done()
Expand All @@ -165,47 +183,53 @@ module.exports = (repo) => {
it('resolver.get relative path `./one/someData` (same as get one/someData)', (done) => {
resolver.get(cid2, './one/someData', (err, result) => {
expect(err).to.not.exist()
expect(result.value).to.eql('I am 1')
expect(result.length).to.eq(2)
expect(result[1].value).to.eql('I am 1')
done()
})
})

it('resolver.get relative path `one/./someData` (same as get one/someData)', (done) => {
resolver.get(cid2, 'one/./someData', (err, result) => {
expect(err).to.not.exist()
expect(result.value).to.eql('I am 1')
expect(result.length).to.eq(2)
expect(result[1].value).to.eql('I am 1')
done()
})
})

it('resolver.get double slash at the beginning `//one/someData` (same as get one/someData)', (done) => {
resolver.get(cid2, '//one/someData', (err, result) => {
expect(err).to.not.exist()
expect(result.value).to.eql('I am 1')
expect(result.length).to.eq(2)
expect(result[1].value).to.eql('I am 1')
done()
})
})

it('resolver.get double slash in the middle `one//someData` (same as get one/someData)', (done) => {
resolver.get(cid2, 'one//someData', (err, result) => {
expect(err).to.not.exist()
expect(result.value).to.eql('I am 1')
expect(result.length).to.eq(2)
expect(result[1].value).to.eql('I am 1')
done()
})
})

it('resolver.get value within 1st node scope', (done) => {
resolver.get(cid1, 'someData', (err, result) => {
expect(err).to.not.exist()
expect(result.value).to.eql('I am 1')
expect(result.length).to.eq(1)
expect(result[0].value).to.eql('I am 1')
done()
})
})

it('resolver.get value within nested scope (0 level)', (done) => {
resolver.get(cid2, 'one', (err, result) => {
expect(err).to.not.exist()
expect(result.value).to.eql({
expect(result.length).to.eq(2)
expect(result[1].value).to.eql({
someData: 'I am 1'
})
done()
Expand All @@ -215,16 +239,18 @@ module.exports = (repo) => {
it('resolver.get value within nested scope (1 level)', (done) => {
resolver.get(cid2, 'one/someData', (err, result) => {
expect(err).to.not.exist()
expect(result.value).to.eql('I am 1')
expect(result.length).to.eq(2)
expect(result[1].value).to.eql('I am 1')
done()
})
})

it('resolver.get value within nested scope (2 levels)', (done) => {
resolver.get(cid3, 'two/one/someData', (err, result) => {
expect(err).to.not.exist()
expect(result.value).to.eql('I am 1')
expect(result.remainderPath).to.eql('')
expect(result.length).to.eq(3)
expect(result[2].value).to.eql('I am 1')
expect(result[2].remainderPath).to.eql('')

done()
})
Expand Down Expand Up @@ -306,7 +332,7 @@ module.exports = (repo) => {
expect(err).to.not.exist()
resolver.get(cid1, (err, result) => {
expect(err).to.not.exist()
expect(node1).to.eql(result.value)
expect(node1).to.eql(result[0].value)
remove()
})
})
Expand Down
Loading