-
Notifications
You must be signed in to change notification settings - Fork 299
Fix for dag.get failing silently on missing multicodec #831
Conversation
68ee648
to
0ba96bf
Compare
src/dag/get.js
Outdated
} | ||
const error = new Error('ipfs-api is missing DAG resolver for "' + ipfsBlock.cid.codec + '" multicodec') | ||
error.missingMulticodec = ipfsBlock.cid.codec | ||
cb(error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, only comment is a stylistic one - usually we return early on error condition, but we're doing the opposite here. Personally I'd if (!dagResolver) return cb(new Error(/*...*/))
.
test/dag.spec.js
Outdated
ipfsd.stop(done) | ||
}) | ||
|
||
it('.dag.put+get dag-pb', (done) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love to see something a little more descriptive here...".dag.put+get dag-pb" is a bit cryptic.
e.g. "should be able to put and get a DAG node with format dag-pb"
test/dag.spec.js
Outdated
}) | ||
}) | ||
|
||
it('.dag.get missing raw multicodec returns error', (done) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a better description, could use a tiny bit more improvement:
e.g. "should callback with error when missing DAG resolver for raw multicodec"
😝
test/dag.spec.js
Outdated
// CIDv1 with multicodec = raw | ||
const cid = 'bafkreigh2akiscaildcqabsyg3dfr6chu3fgpregiymsck7e7aqa4s52zy' | ||
ipfs.dag.get(cid, (err, result) => { | ||
expect(err.message).to.equal('ipfs-api is missing DAG resolver for "raw" multicodec') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(err).to.exist()
before this please :) That way if the error is undefined
/null
we get an error message saying that the error was expected to exist rather than "TypeError: Cannot read property 'message' of null" which is a little harder to debug.
test/dag.spec.js
Outdated
const cbor = {foo: 'dag-cbor-bar'} | ||
ipfs.dag.put(cbor, {format: 'dag-cbor', hashAlg: 'sha2-256'}, (err, cid) => { | ||
expect(err).to.not.exist() | ||
cid = new CID(cid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should already be a CID instance.
test/dag.spec.js
Outdated
expect(err).to.not.exist() | ||
ipfs.dag.put(node, {format: 'dag-pb', hashAlg: 'sha2-256'}, (err, cid) => { | ||
expect(err).to.not.exist() | ||
cid = new CID(cid).toV0() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should already be a CID instance
test/dag.spec.js
Outdated
}) | ||
}) | ||
|
||
it('.dag.put+get dag-cbor', (done) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can we have a more readable description here as well?
Before this change it just failed silently for 'raw' DAGs such as `bafkreigh2akiscaildcqabsyg3dfr6chu3fgpregiymsck7e7aqa4s52zy` License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>
0ba96bf
to
107699d
Compare
@alanshaw applied suggested changes and rebased, ok to merge? |
Right now
ipfs.dag.get
fails silently for CIDs with codecs different thandag-cbor
ordag-pb
.To reproduce, try
dag.get
forraw
one atbafkreigh2akiscaildcqabsyg3dfr6chu3fgpregiymsck7e7aqa4s52zy
.This PR fixes the bug by returning error when codec resolver is missing.
Notes:
dag.put+get
, not sure if it is the right place tho. Any feedback would be appreciated.