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

fix: streaming cat over http api #1760

Merged
merged 2 commits into from
Dec 11, 2018
Merged

fix: streaming cat over http api #1760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
395e96a
fix: streaming cat over http api
Dec 7, 2018
dc09a65
fix: add guard for no content
Dec 11, 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: 14 additions & 7 deletions src/core/components/files-regular.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,21 +232,28 @@ module.exports = function (self) {

pull(
exporter(ipfsPath, self._ipld, options),
pull.filter(filterFile),
pull.take(1),
pull.collect((err, files) => {
if (err) { return d.abort(err) }
if (files && files.length > 1) {
files = files.filter(filterFile)
if (err) {
return d.abort(err)
}
if (!files || !files.length) {

if (!files.length) {
return d.abort(new Error('No such file'))
}

const file = files[0]
const content = file.content
if (!content && file.type === 'dir') {

if (!file.content && file.type === 'dir') {
Copy link
Member

@achingbrain achingbrain Dec 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

These conditions might be simpler as:

Suggested change
if (!file.content && file.type === 'dir') {
if (file.type !== 'file') {
return d.abort(new Error('this dag node was not a file')
}
if (!file.content) {
return d.abort(new Error('this dag node had no content')
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I know that companion depends on the error message "this dag node is a directory" (it is the same for go ipfs) - it is cheaper to call cat and receive this error message than do two calls, one to figure it out. I'd rather not introduce another breaking change for this release (it's really breaky already)!

The interesting bit is a null file.content. When type is file, is that possible? I'll add this test in because that could break the pull pipeline if we resolve the deferred with something that isn't a pull stream.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I guess once all the errors have codes we can stop depending on message content.

If file.type === 'file' then file.content should always have a value. It's weird that there is a check for that in the first place 🤷‍♂️.

return d.abort(new Error('this dag node is a directory'))
}
d.resolve(content)

if (!file.content) {
return d.abort(new Error('this dag node has no content'))
}

d.resolve(file.content)
})
)

Expand Down
55 changes: 35 additions & 20 deletions src/http/api/resources/files-regular.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const toStream = require('pull-stream-to-stream')
const abortable = require('pull-abortable')
const Joi = require('joi')
const ndjson = require('pull-ndjson')
const { PassThrough } = require('readable-stream')

exports = module.exports

Expand Down Expand Up @@ -79,27 +80,41 @@ exports.cat = {
const options = request.pre.args.options
const ipfs = request.server.app.ipfs

ipfs.cat(key, options, (err, stream) => {
if (err) {
log.error(err)
if (err.message === 'No such file') {
reply({ Message: 'No such file', Code: 0, Type: 'error' }).code(500)
} else {
reply({ Message: 'Failed to cat file: ' + err, Code: 0, Type: 'error' }).code(500)
}
return
}
let pusher
let started = false

// hapi is not very clever and throws if no
// - _read method
// - _readableState object
// are there :(
if (!stream._read) {
stream._read = () => {}
stream._readableState = {}
}
return reply(stream).header('X-Stream-Output', '1')
})
pull(
ipfs.catPullStream(key, options),
pull.drain(
chunk => {
if (!started) {
started = true
pusher = pushable()
reply(toStream.source(pusher).pipe(new PassThrough()))
.header('X-Stream-Output', '1')
}
pusher.push(chunk)
},
err => {
if (err) {
log.error(err)

// We already started flowing, abort the stream
if (started) {
return pusher.end(err)
}

const msg = err.message === 'No such file'
? err.message
: 'Failed to cat file: ' + err

return reply({ Message: msg, Code: 0, Type: 'error' }).code(500)
}

pusher.end()
}
)
)
}
}

Expand Down