IPIP-328: JSON and CBOR Response Formats on HTTP Gateways#328
Conversation
0a081f4 to
4134864
Compare
|
@lidel I reached a nice first-draft. There's some sections that are yet incomplete, but I'm marking this as ready for review as it is definitely ready for receiving some feedback. |
|
|
||
| ## Test fixtures | ||
|
|
||
| TODO |
There was a problem hiding this comment.
I think we want at minimum to provide basic test vectors (we want to have them in Kubo tests and only reference CIDs here)
- CID with
dag-pbcodec and a corresponding Logical DAG-JSON representation that is expected to be returned when?format=dag-jsonis passed - CID with
jsoncodec that is a valid JSON but not a valid DAG-JSON - CID with
cborcodec that is a valid CBOR but not a valid DAG-CBOR - CID with
dag-jsoncodec- dag-json document should link to other dag-json, so we can test DAG-traversal
/ipfs/{dag-json-cid}/field-linking-to-other-dag-json/– this should be enough smoke-test
- dag-json document should link to other dag-json, so we can test DAG-traversal
- CID with
dag-cborcodec- dag-cbor document should link to other dag-cbor, so we can test DAG-traversal
/ipfs/{dag-cbor-cid}/field-linking-to-other-dag-cbor/– this should be enough smoke-test
- dag-cbor document should link to other dag-cbor, so we can test DAG-traversal
+ invalid versions
There was a problem hiding this comment.
@lidel Unfortunately, I'm not sure if this will be easy to do. See ipfs/kubo#9335 (comment). It seems that the Go codecs encoders are quite strict, making the differences between both codecs very minimal. I thought that we could play with the same JSON document containing a link: in JSON it won't be resolved, in DAG-JSON it will. Same for CBOR.
There was a problem hiding this comment.
Test fixtures are also welcome at https://github.com/ipld/codec-fixtures :) We especially are missing failing ones. So if you create some, perhaps also add them there.
There was a problem hiding this comment.
@vmx @lidel regarding invalid versions, I'm thinking about what we could do. We would need something that is valid in some format, such as CBOR, but not valid in other format, such as JSON. The problem being that we would need to add the data to IPFS somehow and the serialisation process would already check if the data is valid or not for the codec.
Considering that DAG-JSON and DAG-CBOR are both subsets of their non DAG-* versions, I can't think of anything on top of my mind that would work. We could see if we could add a weird CBOR that would fail when requested via format=json/format=dag-json (as both are equivalent when requesting a CBOR document.
I think this is related to what I mentioned on the previous comment: that the Go impl. of CBOR and JSON are quite strict by themselves, so adding something to IPFS that would then fail when downloading through the gateway would be quite hard. Am I missing anything?
There was a problem hiding this comment.
I have to admit I'm not deep into this whole IPIP, I just read "fixtures" and "DAG-CBOR/JSON" and jumped in :)
Valid JSON, but invalid DAG-JSON is anything with strange things after a property with just a slash, e.g. {"/": true}. Valid CBOR, but invalid DAG-CBOR: tags that are not tag 42. But of course all valid DAG-CBOR/JSON should be valid in the non DAG*-versions. Also valid DAG-JSON and DAG-CBOR should be interchangeable with each other.
Co-authored-by: Marcin Rataj <[email protected]>
Co-authored-by: Marcin Rataj <[email protected]>
Co-authored-by: Marcin Rataj <[email protected]>
Co-authored-by: Marcin Rataj <[email protected]>
e4cde04 to
a0c86bb
Compare
|
@lidel I have addressed most of your feedback and I think the IPIP is quite complete. I'm going to work on figuring out if I can find any JSON/CBOR that is invalid DAG-JSOn and DAG-CBOR. As I mentioned in the comment, I think that might be a bit difficult, specially since some of the constraints of DAG-JSON are also present in Go's JSON implementation (large numbers for example). |
#9335 ipfs/specs#328 Co-authored-by: Marcin Rataj <[email protected]>
|
We've discussed this during IPFS-Implementers-Sync-2022-12-08 and no concerns were raised, @b5 (Iroh) +1'd, decision was made to ratify this. I'll merge this PR when a stable Kubo 0.18 ships with the implementation. |
3e5fbc7 to
e4cbfd1
Compare
|
@lidel I think the IPIP explains the current behaviour well and can be safely merged as Kubo 0.18 has been shipped with this functionality. |
ipfs/kubo#9335 ipfs/specs#328 Co-authored-by: Marcin Rataj <[email protected]> This commit was moved from ipfs/kubo@fdd1965
|
My understanding is the remaining step is for @lidel to give a last read through and merge. |
|
Thank to @hacdias and everyone involved in this over the years. For drive-by-reader: reference implementation of this IPIP shipped in Kubo 0.18 |
Uh oh!
There was an error while loading. Please reload this page.