Skip to content

Commit

Permalink
fix: do not convert existing daglinks
Browse files Browse the repository at this point in the history
  • Loading branch information
dignifiedquire committed May 16, 2016
1 parent 04ee50a commit 0e4361f
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 7 deletions.
16 changes: 9 additions & 7 deletions src/dag-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,17 @@ module.exports = class DAGNode {

// ensure links are instances of DAGLink
if (links) {
links.map((l) => {
return {
size: l.size || l.Size,
name: l.name || l.Name,
hash: l.hash || l.Hash
links.forEach((l) => {
if (l.name && typeof l.toJSON === 'function') {

This comment has been minimized.

Copy link
@AdamStone

AdamStone May 19, 2016

I'm hitting an error here when calling new DAGNode(data, links) if links includes DAGLink objects with empty names, as they are not recognized as DAGLink objects. Are link names allowed to be null or empty string? Based on the go implementation I thought this was the case. What about if (l instanceof DAGLink) instead?

This comment has been minimized.

Copy link
@daviddias

daviddias May 19, 2016

Member

instanceof is typically a double edge sword cause if there are several versions of the same object, even with minimal diferences, the check will fail.

Nevertheless, it should be possible to have links with no name. @dignifiedquire did you had something else in mind with this check?

This comment has been minimized.

Copy link
@dignifiedquire

dignifiedquire May 19, 2016

Author Member

I was under the impression that links had to have names that's why I coded it in this way. If that's not the case we have to adjust this. Will check the go code later for details

This comment has been minimized.

Copy link
@daviddias

daviddias May 19, 2016

Member

at minimum, they can be empty strings. What is important to check is that if a DAGLink with a empty name is a empty string or a undefined, because that will change the protobuf. Let's add a test for that by comparing a byte by byte

this.links.push(l)
} else {
this.links.push(
new DAGLink(l.Name, l.Size, l.Hash)
)
}
}).forEach((l) => {
this.addRawLink(l)
})

stable.inplace(this.links, linkSort)
}
}

Expand Down
1 change: 1 addition & 0 deletions test/dag-node-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ module.exports = function (repo) {
const d2 = new DAGNode(new Buffer('some data'), l2)
expect(d1.toJSON()).to.be.eql(d2.toJSON())
expect(d1.marshal()).to.be.eql(d2.marshal())
expect(d2.links).to.be.eql(l2)
})

it('create an emtpy node', function (done) {
Expand Down

0 comments on commit 0e4361f

Please sign in to comment.