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

fix(gateway): undesired conversions to dag-json and friends #9566

Merged
merged 9 commits into from
Jan 21, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jan 19, 2023

In the spirit of https://github.com/protocol/bifrost-infra/issues/2290 and ipfs/specs#328 (review), this PR removes implicit conversion from UnixFS/RAW to DAG-* when json is requested, and performs the conversion only when expliti dag-json is present in the request.

We also fixed handling of CID with json and cbor codecs + added more tests.

Weird, unrelated things, are happening in the CI. – tracked in #9570

@hacdias
Copy link
Member Author

hacdias commented Jan 19, 2023

@lidel another option I see is to completely remove support for ?format=json|cbor but that will also raise the question of: should we then also support CIDs whose codec is JSON|CBOR?

@BigLep BigLep mentioned this pull request Jan 19, 2023
@lidel
Copy link
Member

lidel commented Jan 19, 2023

Let's add a test that ensures we keep read-only support for CIDs with json and cbor codec: when someone requests /ipfs/bagaaierayeva7pcl4bchk3udf3i5qm3eeqlbj6tzcmidv64uonglmi5nebca (CID with json codec) without any parameters, we want gateway to return same payload as raw block response, but with content-type: application/json based on the codec from CID.

@hacdias
Copy link
Member Author

hacdias commented Jan 19, 2023

@lidel we had the tests already 😃 (test_cmp_dag_get).

@lidel lidel self-assigned this Jan 19, 2023
@lidel
Copy link
Member

lidel commented Jan 20, 2023

This is getting really hairy. I am writing more tests, as I've found some new papercuts and missing cases that we should have regression tests for:

  • Requesting CID with json codec with Accept: aplication/json should work, but does not
  • Requesting CID with dag-json codec with Accept: aplication/json should work (without conversion of bytes), but does not

I will prepare a fix and push together with tests.

- adds bunch of additional tests including JSON file on UnixFS
- fix: dag-json codec (0x0129) can be returned as plain json
- fix: json codec (0x0200) cna be retrurned as plain json
- same for cbor variants
@lidel
Copy link
Member

lidel commented Jan 20, 2023

Pushed two fixes and tests in c3ebe80
It should solve the problem from https://github.com/protocol/bifrost-infra/issues/2290 and be ready for 0.18.

@hacdias when you start on Friday, take a look at the above changes as a sanity check and lmk if anything is weird or needs to be better commented.

(interop/webui tests are failing due to unrelated reason – tracking in #9570)

@hacdias
Copy link
Member Author

hacdias commented Jan 20, 2023

@lidel what you pushed works if a file has JSON content (or whatever content it has). However, it does not work for a CID that has the codec JSON (or CBOR for that matter). The fault in the tests lies in that FILE_JSON_CID is just a regular UnixFS/Raw CID and not a JSON CID.

I pushed a fix and another test... I'm also not very happy with this. It's becoming increasingly complex and I feel like there must be a way of simplifying this. I will give it some thought during today's morning and see if I can simplify it somehow.

@lidel
Copy link
Member

lidel commented Jan 20, 2023

Thanks for catching this @hacdias

Maybe we could simplify things a bit by adding CID codec check at the end of customResponseFormat func, and set explicit one when CID is json cbor dag-json or dag-cbor? at least that will happen it one place.

I will circle back to this PR after meetings, but don't want to block the release, we have test coverage now which is the most important part.

We may:

I feel this PR makes it safe to ship in 0.18, but lmk

@lidel lidel mentioned this pull request Jan 20, 2023
2 tasks
@hacdias
Copy link
Member Author

hacdias commented Jan 20, 2023

@lidel I think we should merge as-is and simplify later if possible.

@lidel lidel changed the title fix(gateway): do not convert unixfs/raw into dag-* unless explicit fix(gateway): undesired conversions to dag-json and friends Jan 20, 2023
@BigLep
Copy link
Contributor

BigLep commented Jan 20, 2023

I'm good with us doing the followups in the next release (assuming we do the changelog update and have the followup issue for 0.19). Thanks for handling.

@lidel
Copy link
Member

lidel commented Jan 20, 2023

(writing updates to release notes section, will push and merge before Monday)

@lidel lidel merged commit c706c63 into master Jan 21, 2023
@lidel lidel deleted the json-cbor-changes-2 branch January 21, 2023 03:21
galargh pushed a commit that referenced this pull request Jan 23, 2023
* fix(gateway): do not convert unixfs/raw into dag-* unless explicit
* fix(gateway): keep only dag-json|dag-cbor handling
* fix: allow requesting dag-json as application/json
- adds bunch of additional tests including JSON file on UnixFS
- fix: dag-json codec (0x0129) can be returned as plain json
- fix: json codec (0x0200) cna be retrurned as plain json
* fix: using ?format|Accept with CID w/ codec works
* docs(changelog): cbor and json on gateway

Co-authored-by: Marcin Rataj <lidel@lidel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants