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

IPIP-328: JSON and CBOR Response Formats on HTTP Gateways #328

Merged
merged 50 commits into from
Feb 13, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Oct 7, 2022

@hacdias
Copy link
Member Author

hacdias commented Oct 18, 2022

@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.

@hacdias hacdias marked this pull request as ready for review October 18, 2022 11:53
@hacdias hacdias requested review from lidel and a team as code owners October 18, 2022 11:53
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@hacdias thank you! Did a quick pass with early feedback which should help you with next steps here.

http-gateways/PATH_GATEWAY.md Outdated Show resolved Hide resolved
http-gateways/PATH_GATEWAY.md Show resolved Hide resolved
http-gateways/PATH_GATEWAY.md Outdated Show resolved Hide resolved
IPIP/0000-gateway-json-cbor-response-format.md Outdated Show resolved Hide resolved
IPIP/0000-gateway-json-cbor-response-format.md Outdated Show resolved Hide resolved
IPIP/0000-gateway-json-cbor-response-format.md Outdated Show resolved Hide resolved
IPIP/0000-gateway-json-cbor-response-format.md Outdated Show resolved Hide resolved
IPIP/0000-gateway-json-cbor-response-format.md Outdated Show resolved Hide resolved
IPIP/0000-gateway-json-cbor-response-format.md Outdated Show resolved Hide resolved

## Test fixtures

TODO
Copy link
Member

@lidel lidel Oct 19, 2022

Choose a reason for hiding this comment

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

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-pb codec and a corresponding Logical DAG-JSON representation that is expected to be returned when ?format=dag-json is passed
  • CID with json codec that is a valid JSON but not a valid DAG-JSON
  • CID with cbor codec that is a valid CBOR but not a valid DAG-CBOR
  • CID with dag-json codec
    • 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
  • CID with dag-cbor codec
    • 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

+ invalid versions

Copy link
Member Author

@hacdias hacdias Oct 20, 2022

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

@hacdias hacdias force-pushed the feat/gateway-json-cbor branch from e4cde04 to a0c86bb Compare October 20, 2022 10:04
@hacdias hacdias requested a review from lidel October 20, 2022 10:17
@hacdias
Copy link
Member Author

hacdias commented Oct 20, 2022

@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).

@hacdias hacdias requested a review from lidel November 28, 2022 14:36
@hacdias hacdias requested a review from lidel November 28, 2022 14:50
@hacdias hacdias changed the title IPIP-328: JSON and CBOR Gateway Response Formats IPIP-328: JSON and CBOR Response Formats on HTTP Gateways Nov 29, 2022
lidel added a commit to ipfs/kubo that referenced this pull request Dec 5, 2022
@hacdias
Copy link
Member Author

hacdias commented Dec 6, 2022

I reworked the IPIP in order to make it more about CBOR Tag 42, CBOR and JSON, than IPLD. /cc @lidel @b5

@lidel
Copy link
Member

lidel commented Dec 8, 2022

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.

http-gateways/PATH_GATEWAY.md Outdated Show resolved Hide resolved
http-gateways/PATH_GATEWAY.md Outdated Show resolved Hide resolved
@hacdias hacdias requested a review from lidel January 24, 2023 08:55
@hacdias
Copy link
Member Author

hacdias commented Jan 24, 2023

@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.

hacdias added a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
ipfs/kubo#9335
ipfs/specs#328

Co-authored-by: Marcin Rataj <lidel@lidel.org>

This commit was moved from ipfs/kubo@fdd1965
@BigLep
Copy link
Contributor

BigLep commented Jan 31, 2023

My understanding is the remaining step is for @lidel to give a last read through and merge.

@lidel
Copy link
Member

lidel commented Feb 13, 2023

Thank to @hacdias and everyone involved in this over the years.
Honorary mention to @alanshaw who waited long time to see this since his original PR 🙈 🥳

For drive-by-reader: reference implementation of this IPIP shipped in Kubo 0.18

@lidel lidel merged commit 9141da7 into main Feb 13, 2023
@lidel lidel deleted the feat/gateway-json-cbor branch February 13, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ratified
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants