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
Add endpoints output: improvements and compliance #569
Conversation
@@ -390,9 +389,9 @@ func outputDagnode(out chan *api.AddedOutput, name string, dn ipld.Node) error { | |||
} | |||
|
|||
out <- &api.AddedOutput{ | |||
Hash: dn.Cid().String(), | |||
Cid: dn.Cid().String(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this change will require update (PR) to js-ipfs-api, which still expects Hash
in file-result-stream-converter.js#L32 and L38 (it should try Hash
and if missing use Cid
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey, this only affects the Cluster API /add. The IPFS Proxy /add output will be fully compatible with the ipfs output format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I get it now, thanks for explanation!
This straigthens some mistakes with the outputs of the /add endpoints. Currently, we had exactly the same output format which: * was not exactly the ipfs API output format but was sort of similar * made some weird concessions to be compatible (like having a string-type "size") * was not aligned with Cluster API conventions (lowercase keys) This corrects all this: * The Cluster API /add output format now uses the right types and lowercase keys. * `Hash` is now `Cid`, because the field carries a Cid. * We copy error handling with request trailers from IPFS, and avoid carrying the errors in the output objects. * The proxy now returns exactly the types as ipfs would * We add the X-Chunked-Output: 1 header, which is custom and redundant, but otherwise breaks js-ipfs-api integrations with the /add endpoint. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
8fdd0c4
to
b6306a6
Compare
This straigthens some mistakes with the outputs of the /add endpoints.
Currently, we had exactly the same output format which:
This corrects all this:
Hash
is nowCid
, because the field carries a Cid.errors in the output objects.
otherwise breaks js-ipfs-api integrations with the /add endpoint.
Fixes: ipfs/ipfs-companion#600
License: MIT
Signed-off-by: Hector Sanjuan code@hector.link