file.cat API not supporting CIDs

[mitra42]

• Version: 0.27.7
• Platform: OSX
• Subsystem: files

Type: Bug

Severity: High

Description:

https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/FILES.md#cat
Says that in ipfs.files.cat(ipfsPath, cb)
That ipfsPath can be a CID though it says it should be a “raw Buffer of the cid” or “String base58"

We have been calling files.cat with the CID data structure for a while,
As reported by Chrome debugging ….
CID {codec: "dag-pb", version: 0, multihash: Uint8Array(34)}

but my latest update of node packages seems to have broken something… and I’m seeing an error. I could be wrong , but don’t think we were hitting this error before, though its possible we were not using that part of the code.

Error: path.indexOf is not a function …

Looking at the code, it only handles Buffers or strings, not CIDs.

function normalizePath (path) {
  if (Buffer.isBuffer(path)) {  path = toB58String(path) }
  if (path.indexOf('/ipfs/') === 0) { path = path.substring('/ipfs/'.length) }
  if (path.charAt(path.length - 1) === '/') { path = path.substring(0, path.length - 1) }
  return path
}

Stack Trace

_catPullStream
cat.promisify
Anonymous
Anonymous
 await this.ipfs.files.cat(cid)

I wanted to check if this was an IPFS bug, or whether I should convert all my CIDs back to base58 strings if necessary.

I had thought the direction was towards using CIDs rather than strings or buffers so wanted to check here first before making those edits. If so I’d appreciate what is the “best” way to turn a CID into something to pass to ipfs.files.cat.

[daviddias]

ipfs.files.cat expects a valid IPFS Path, which includes CID. However, I believe that this was never supported, I don't find anywhere in the githistory where the normalize path func checked for CIDs. Good catch!

Would you like to contribute with a failing test to the batch here -- https://github.com/ipfs/interface-ipfs-core/blob/master/js/src/files.js -- so that we make sure to fix it and not let it be reverted?

Thanks!

[achingbrain]

This test sounds like what @mitra42 is trying to do:

const CID = require('cids')

it('cats with a cid object', (done) => {
  const cid = new CID(smallFile.cid)

  ipfs.files.cat(cid, (err, data) => {
    expect(err).to.not.exist()
    expect(data.toString()).to.contain('Plz add me!')
    done()
  })
})

The spec does not say you can use instances of CID as the ipfsPath argument though so the failure looks valid (although the error message could be better).

It sounds like something that should be supported, should the spec be updated and this be implemented?

[daviddias]

Thanks @achingbrain :)

It sounds like something that should be supported, should the spec be updated and this be implemented?

Yes, that is the right next step. Thanks!

[realharry]

Hi, I'm an absolute newbie to ipfs. I'd like to tackle one of the issues here to get started. This issue #1229 seems to be straightforward (in my untrained eyes). Is this a good problem for a beginner to look into? @diasdavid Would you have any different suggestions as the first problem for a person with no/little experience with the js-ipfs codebase? Thanks!

[daviddias]

@realharry it is indeed a good first issue. It basically just needs for the type to be checked and pass the translate it to the correct format for Unixfs engine.

[mitra42]

@realharry - it might also be worth looking at older versions of IPFS, I just looked at one of my test scripts which use to run, and it used to use cid's ipfs.files.cat(cid) but now fails.

[jacobheun]

Created a PR to add CID instances to the SPEC: ipfs-inactive/interface-js-ipfs-core#243
PR for the functionality to the api: ipfs-inactive/js-ipfs-http-client#721
PR for the functionality here: #1275