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

gateway: implement IPIP-0445 (skip-raw-blocks option) #502

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Nov 2, 2023

@Jorropo
Copy link
Contributor Author

Jorropo commented Nov 2, 2023

Need tests merged and changelog.

@@ -296,6 +296,10 @@ func (bb *BlocksBackend) Head(ctx context.Context, path path.ImmutablePath) (Con
var emptyRoot = []cid.Cid{cid.MustParse("bafkqaaa")}

func (bb *BlocksBackend) GetCAR(ctx context.Context, p path.ImmutablePath, params CarParams) (ContentPathMetadata, io.ReadCloser, error) {
if params.SkipRawBlocks.Bool() && p.RootCid().Prefix().Codec == cid.Raw {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs 400:

A Gateway MUST return HTTP error 400 Bad Request when skip-raw-blocks=y is sent for a content path with a root CID with the raw multicodec.

Comment on lines +784 to +793
// FIXME(@Jorropo): this is very inneficient, we fetch all blocks even if we don't send them.
// I've tried doing so using the ipld stack however the problem is that filtering on the
// selector or traversal callback does not work because the unixfs reifier is ran before,
// so trying to filter raw links do nothing because go-unixfsnode removed them already,
// so we need to filter in a callback from unixfsnode but the reifier does not know a about
// [traversal.SkipMe] making it a lost cause. I've looked into updating unixfsnode but this
// much more work because there are no easy way to pass options or understand what the side
// effects of this would be.
// Abstractions everywhere yet a simple small behaviour change require rethinking everything :'(.
// Will fix with boxo/unixfs.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

😞

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #502 (f7bb4b3) into main (9c22812) will decrease coverage by 0.09%.
The diff coverage is 32.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #502      +/-   ##
==========================================
- Coverage   65.66%   65.58%   -0.09%     
==========================================
  Files         207      207              
  Lines       25604    25656      +52     
==========================================
+ Hits        16814    16827      +13     
- Misses       7317     7353      +36     
- Partials     1473     1476       +3     
Files Coverage Δ
gateway/blocks_backend.go 43.24% <59.09%> (+0.52%) ⬆️
gateway/handler_car.go 74.74% <9.09%> (-3.87%) ⬇️
gateway/gateway.go 74.66% <17.39%> (-10.38%) ⬇️

... and 12 files with indirect coverage changes

@BigLep BigLep requested a review from aschmahmann November 2, 2023 16:20
@BigLep BigLep mentioned this pull request Nov 9, 2023
11 tasks
@lidel lidel changed the title gateway: implement ipip-0445 gateway: implement IPIP-0445 (skip-raw-blocks option) Nov 9, 2023
@Jorropo Jorropo removed their assignment Mar 4, 2024
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
Status: 🔎 In Review
Development

Successfully merging this pull request may close these issues.

1 participant