Response body is empty if X-Chunked-Output header is set

[vmx]

When the X-Chunked-Output: 1 header is set, then the body of a HTTP request isn't passed along correctly.

Line 34 in send-request.js it's responsible for a different code path is X-Chunked-Output: 1 is set. This relates to ipfs/kubo#4721.

If this code path is taken then there's no streamToJsonValue transformation taken (https://github.com/ipfs/js-ipfs-api/blob/de8d97c953447b605e2a1de68d198b5372419724/src/utils/send-request.js#L68). The send() at https://github.com/ipfs/js-ipfs-api/blob/de8d97c953447b605e2a1de68d198b5372419724/src/utils/send-files-stream.js#L112-L148 has a DestroyableTransform as response. When the code reaches line 113, it does contain the expected body. But that body doesn't make it to here: https://github.com/maxogden/concat-stream/blob/e482281642c1e011fc158f5749ef40a71c77a426/index.js#L36.

As opposed to the code path when X-Chunked-Output is not set. Then the body at https://github.com/maxogden/concat-stream/blob/e482281642c1e011fc158f5749ef40a71c77a426/index.js#L36 contains the expected value.

[vmx]

If this is too vague, I've pushed a branch, showing that issue: https://github.com/ipfs/js-ipfs-api/tree/issue-696

Check it out and run:

npx mocha --inspect-brk --timeout 5000 --ui bdd --colors --exit test/interface/dag.spec.js

Compare those code paths with (where things work as expected):

npx mocha --inspect-brk --timeout 5000 --ui bdd --colors --exit test/interface/block.spec.js

The X-Chunked-Output: 1 code path is there for a long time, so it could well be some legacy. I've not really a clue what the real issue is and how it should be fixed. @diasdavid can you please help?

[daviddias]

@pgte will have a clear answer here as he refactored the send-request.js code last. If not I'm happy to dive deep into this with you.

[pgte]

on it.

[pgte]

@vmx I think I know what the problem is.
send-files-stream.js pipes the response through a converter stream, which is expecting a response like the one from the files api (expects objects with the shape { Name, Hash, Size }). These sets of utils function are only used in the files API.

Perhaps you could, instead of using a file-specific API, try using directly the utils/send-request.js directly, which, in the case of dag.put, should return an object stream, which you can then concat, verify there is exactly one response object, and retrieve that last object.

@vmx Makes sense?

[vmx]

@pgte Thanks for the quick reply, it makes sense.

block/put returns just {Name, Size}, which seems to be good enough. I wonder if the API should be more uniform and dag/put should also return {Name, Size}?

Could the converter be renamed? Just "converter" sounds so general purpose (hence I didn't really spot it as an issue). Would it make sense to rename it to something like IpfsFileConverter, FileConverter, FileDataConverter, FileApiConverter, NameHashSizeCoverter or something similar?

[pgte]

@vmx Agree, should be renamed for clarity, I would say something like FileResponseConverter?

[vmx]

@pgte I had another look. So block/put doesn't return {Name, Size}, but {Key, Size}. Is it correct, that if the X-Chunked-Output header is not set, then the converter isn't run? Is it intentional or an inconsistency?

[pgte]

Well, converter should be called converter-stream. It's used to filter out progress values (which feed the progress bar) from the user.

X-Chunked-Output signals a streaming output, but if converter gets renamed to something like FileResponseConverterStream, I think this starts making sense.. :)

[vmx]

So converter renames fields, converts types and filters out things from a stream (btw, the filtering is not mentioned on in the comments).

Does this mean send-one-file and send-one-file-multiple-results should only be used if the response is a stream? Or should they even only be used with the file related endpoints? I'm asking because block/put is also using SendOneFile which I wouldn't consider a file related endpoint.

Please don't get me wrong. My goal is to take the chance that I'm new to the code base to clean up the API a bit to make it easier to follow/understand/use.

[pgte]

@vmx good point, you're absolutely right!
Hmm... I see 3 options here:

1. Take out converter, no longer filtering the response. This leaves specific endpoints having to create a FileConverterStream and pipe it.
2. Each endpoint opts in into the converter somehow, as an option or an extra builder
3. Each endpoint injects or specifies the converter to use (if any)

@vmx thoughts?

[vmx]

@pgte I would follow Python's "explicit is better than implicit". That would mean that each endpoint would add its converter as an option.

[pgte]

@vmx I'm all for it 👍, I also prefer explicit rather than these nasty surprises..

[vmx]

@pgte Will you be working on this?

[pgte]

@vmx I didn't plan to, but I can help you review this is you're interested..

[MichaelMure]

Is there a fix incoming for this issue ? It's going to be blocking for me quite soon.

[vmx]

@MichaelMure Yes, the fix is at #701. Things are currently kind of blocked due to the CI failing. Getting the tests working again is currently my top priority.

[MichaelMure]

Awesome, thanks :)