Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The documented JSON output for /api/v0/dag/import is incorrect #1980

Open
smoyer64 opened this issue Jan 15, 2025 · 2 comments
Open

The documented JSON output for /api/v0/dag/import is incorrect #1980

smoyer64 opened this issue Jan 15, 2025 · 2 comments
Labels
help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up topic/docs Documentation

Comments

@smoyer64
Copy link

smoyer64 commented Jan 15, 2025

The current documentation describes /api/v0/dag/import as returning a JSON body with the following structure:

{
  "Root": {
    "Cid": {
      "/": "<cid-string>"
    },
    "PinErrorMsg": "<string>"
  },
  "Stats": {
    "BlockBytesCount": "<uint64>",
    "BlockCount": "<uint64>"
  }
}

What is actually returned is a JSONL body with the following structure:

{"Root":{"Cid":{"/":"<cid-string>"},"PinErrorMsg":"string"}}
{"Stats":{"BlockCount":<uint64>,"BlockBytesCount":<uint64>}}

Note that the Stats "record" is only returned if the stats=true argument is provided.

@smoyer64 smoyer64 added the need/triage Needs initial labeling and prioritization label Jan 15, 2025

This comment has been minimized.

smoyer64 added a commit to selesy/ipfs-docs that referenced this issue Jan 15, 2025
@lidel lidel added help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up topic/docs Documentation and removed need/triage Needs initial labeling and prioritization labels Jan 16, 2025
@lidel
Copy link
Member

lidel commented Jan 16, 2025

@smoyer64 thank you for filling this.

As @2color noted, these docs are generated by code mentioned in #1981 (comment)

Your analysis from #1981 (comment) is spot-on, yes, it is intentional.

I think the only missing piece of the puzzle is streaming and why we have it and that CLI uses it by default. Some context below:

  • ipfs CLI makes requests with streaming enabled for all commans by default ( --stream-channels=true is the implicit default and it makes HTTP client append &stream-channels=true to RPC requests).

  • This makes RPC endpoint respond in streaming fashion (NDJSON-like), flushing after every object, allowing server to not have to buffer everything in memory before returning result, and client to consume and react to partial results before entire request is processed.

    Click to expand `ipfs dag import` example

    You can see what happens on the wire bit better with some mitm like socat:

    $ echo -n "hello" | ipfs block put --pin=true
    $ ipfs dag export bafkreibm6jg3ux5qumhcn2b3flc3tyu6dmlb4xa7u5bf44yegnrjhc4yeq > hello.car
    $ ipfs dag import --pin-roots=true --stats --api /dns/localhost/tcp/7423 hello.car

    And then socat proxy shows what happens on the wire bit better:

    $ socat -v TCP-LISTEN:7423,fork TCP:localhost:5001
    [... sending CAR ... ]
    < 2025/01/16 22:48:40.000221008  length=566 from=0 to=565
    HTTP/1.1 200 OK\r
    Access-Control-Allow-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length\r
    Access-Control-Expose-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length\r
    Content-Type: application/json\r
    Server: kubo/0.34.0-dev\r
    Trailer: X-Stream-Error\r
    Vary: Origin\r
    X-Chunked-Output: 1\r
    X-Fookey: BarVal\r
    Date: Thu, 16 Jan 2025 21:48:40 GMT\r
    Connection: close\r
    Transfer-Encoding: chunked\r
    \r
    66\r
    {"Root":{"Cid":{"/":"bafkreibm6jg3ux5qumhcn2b3flc3tyu6dmlb4xa7u5bf44yegnrjhc4yeq"},"PinErrorMsg":""}}
    \r
    2f\r
    {"Stats":{"BlockCount":1,"BlockBytesCount":5}}
    \r
    < 2025/01/16 22:48:40.000221600  length=5 from=566 to=570
    0\r
    \r
  • In practice, you always want to use streaming if the command may return more than one result object. Commands that require streaming like ping or dag import probably do not even allow you to disable streaming.

The docs for ipfs dag import (and all other commands) generated by /tools/http-api-docs seem to miss the concept of streaming in general, and all possible return objects are wrapped in parent {}.

Next steps

With that context in mind, there are probably various ways of fixing this in /tools/http-api-docs generator.

  • We could add a note that when streaming is used, wrapping {} is not present, and objects are separated by \n.

  • We could add a StreamingExample flag to https://github.com/ipfs/go-ipfs-cmds similar to Status and annotate commands like dag import to indicate that streaming line-separated output should be included in addition to single JSON?

  • Something smarter?

Ideas / PRs against /tools/http-api-docs welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up topic/docs Documentation
Projects
None yet
Development

No branches or pull requests

2 participants