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

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b2d9fe
Show which link does not exist on ipfs.cat
youngnicks Mar 18, 2018
87ab4bb
Determine which node in path is missing.
youngnicks Mar 19, 2018
aa2d6a6
Add test for cat nested error.
youngnicks Mar 20, 2018
4f3ec22
Change cat error to match go-ipfs
youngnicks Mar 21, 2018
8b6f79b
More specific cat errors.
youngnicks Mar 21, 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
21 changes: 17 additions & 4 deletions src/core/components/files.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,22 +131,35 @@ 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) => {
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'))
const hash = toB58String(bestMatch.hash)
const link = pathComponents[bestMatch.depth + 1]
return d.abort(new Error(`no link named "${link}" under ${hash}`))
}

const file = files[0]
Expand Down
26 changes: 26 additions & 0 deletions test/cli/files.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,33 @@ describe('files', () => runOnAndOff((thing) => {
return ipfs('cat QmPZ9gcCEpqKTo6aq61g2nXGUhM4iCL3ewB6LDXZCtioEB/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 link named "dummy" under QmPZ9gcCEpqKTo6aq61g2nXGUhM4iCL3ewB6LDXZCtioEB'
)
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.

})
})

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 link named "wrong-dir" under QmegvLXxpVKiZ4b57Xs1syfBVRd8CbucVHAp7KpLQdGieC'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect the error to say something like either:

  • "no file named "wrong-dir" under QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2/init-docs/docs" or
  • "no file named "init-docs/docs/wrong-dir" under QmYmW4HiZhotsoSqnv2o1oUusvkRM8b9RweBoH7ao5nki2" or

Would any of these be possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

Notice that I'm using the term file and not link, on purpose ;)

)
})
})

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 link named "dummy" under QmegvLXxpVKiZ4b57Xs1syfBVRd8CbucVHAp7KpLQdGieC'
)
})
})

Expand Down