Skip to content
This repository was archived by the owner on Feb 12, 2024. It is now read-only.

feat: show which link does not exist on ipfs.cat #1270

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
39 changes: 35 additions & 4 deletions src/core/components/files.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,22 +131,53 @@ module.exports = function files (self) {
throw new Error('You must supply an ipfsPath')
}

let bestMatch = { depth: -1 }

ipfsPath = normalizePath(ipfsPath)
const pathComponents = ipfsPath.split('/')
const prePath = normalizePath(pathComponents.slice(0, 1).join('/'))
const restPath = normalizePath(pathComponents.slice(1).join('/'))
const filterFile = (file) => (restPath && file.path === restPath) || (file.path === ipfsPath)
const filterFile = (file) => {
// Save off best match to provide a better error message if file
// isn't found.
if (file.path === ipfsPath.substring(0, file.path.length)) {
if (file.depth > bestMatch.depth) {
bestMatch = file
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of replacing, could we retain the complete path of the best match to support more relevant error messages? (See my previous comment)

Copy link
Author

Choose a reason for hiding this comment

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

I think replacing here is still okay as the depth tells us how many nodes bestMatch matches and we can use pathComponents to build the path. This is essentially what I was doing earlier on in the pull request. I thought of defining if a directory or file was missing, but it wasn't part of the spec so I left it the way it was.

We could have another scenario where a user tries to specify a file under a file thinking it is a directory. What do you think of these errors?

Missing directory
no directory named "wrong-dir" under QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs

Missing file
no file named "does-not-exist" under QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs

File instead of directory
"actually-a-file" is a file not a directory under QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs

Copy link
Contributor

Choose a reason for hiding this comment

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

@youngnicks this would be even better! 👍

}
}

return (restPath && file.path === restPath) || (file.path === ipfsPath)
}

const d = deferred.source()

pull(
exporter(ipfsPath, self._ipld),
exporter(prePath, self._ipld),
pull.collect((err, files) => {
if (err) { return d.abort(err) }
if (files && files.length > 1) {
if (files) {
files = files.filter(filterFile)
}
if (!files || !files.length) {
return d.abort(new Error('No such file'))
// File used as directory
if (bestMatch.type === 'file') {
const path = bestMatch.path.substring(0, bestMatch.path.length - bestMatch.name.length - 1)
return d.abort(new Error(
`"${bestMatch.name}" is a file not a directory under ${path}`
))
}

const link = pathComponents[bestMatch.depth + 1]

// Missing directory
if (bestMatch.depth < pathComponents.length - 2) {
return d.abort(new Error(
`no directory named "${link}" under ${bestMatch.path}`
))
}

// Missing file
return d.abort(new Error(`no file named "${link}" under ${bestMatch.path}`))
}

const file = files[0]
Expand Down
39 changes: 38 additions & 1 deletion test/cli/files.js
Original file line number Diff line number Diff line change
Expand Up @@ -319,10 +319,47 @@ describe('files', () => runOnAndOff((thing) => {
})

it('cat non-existent file', () => {
return ipfs('cat QmPZ9gcCEpqKTo6aq61g2nXGUhM4iCL3ewB6LDXZCtioEB/dummy')
return ipfs('cat QmUhUuiTKkkK8J6JZ9zmj8iNHPuNfGYcszgRumzhHBxEEU/dummy')
.then(() => expect.fail(0, 1, 'Should have thrown an error'))
.catch((err) => {
const message = err.stderr.match(/^Error:(?: Failed to cat file: Error:)? (.*)$/m)[1]
expect(err).to.exist()
expect(message).to.eql(
'no file named "dummy" under QmUhUuiTKkkK8J6JZ9zmj8iNHPuNfGYcszgRumzhHBxEEU'
)
})
})

it('cat specifies missing directory in a nested link', () => {
return ipfs('cat QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs/wrong-dir/dummy')
.then(() => expect.fail(0, 1, 'Should have thrown an error'))
.catch((err) => {
const message = err.stderr.match(/^Error:(?: Failed to cat file: Error:)? (.*)$/m)[1]
expect(message).to.eql(
'no directory named "wrong-dir" under QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs'
)
})
})

it('cat specifies missing file in a nested link', () => {
return ipfs('cat QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs/dummy')
.then(() => expect.fail(0, 1, 'Should have thrown an error'))
.catch((err) => {
const message = err.stderr.match(/^Error:(?: Failed to cat file: Error:)? (.*)$/m)[1]
expect(message).to.eql(
'no file named "dummy" under QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs'
)
})
})

it('cat specifies a link is not a directory', () => {
return ipfs('cat QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs/index/dummy')
.then(() => expect.fail(0, 1, 'Should have thrown an error'))
.catch((err) => {
const message = err.stderr.match(/^Error:(?: Failed to cat file: Error:)? (.*)$/m)[1]
expect(message).to.eql(
'"index" is a file not a directory under QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs'
)
Copy link
Member

Choose a reason for hiding this comment

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

This feature is still not complete. Please add tests for multiple levels of nestedness (i.e /a/b/c/d) and you will see that after the first level, this PR fails.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I was more asking if this would be an acceptable location for this logic, knowing that it could possibly have a performance hit when there is a large amount of files on a node.

What should the error message show when a middle level is missing? I am thinking the message should be no link named "c/d" under a/b to provide more context to as to what the requested file is.

})
})

Expand Down