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

feat: switch gateway GetRange API to match Get #240

Closed
wants to merge 1 commit into from

Conversation

aschmahmann
Copy link
Contributor

@aschmahmann aschmahmann commented Mar 30, 2023

Also fixes serving range requests for /ipfs/bafydir when we know we'll get the index.html or _redirect.

cc ipfs-inactive/bifrost-gateway#61

Also fixes serving range requests for /ipfs/bafydir when we know we'll
get the index.html or _redirect.
// data, however consumers of this function may require extra data beyond the passed ranges (e.g. the initial bit of
// the file might be used for content type sniffing even if only the end of the file is requested).
// Note: there is currently no semantic meaning attached to a range request for a directory
GetRange(context.Context, ImmutablePath, ...GetRange) (ContentPathMetadata, *GetResponse, error)
Copy link
Contributor Author

@aschmahmann aschmahmann Mar 30, 2023

Choose a reason for hiding this comment

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

Looking at the code now, should we just have a single function Get(context.Context, ImmutablePath, ...GetRange) (ContentPathMetadata, *GetResponse, error) and drop GetRange entirely (since passing no ranges is equivalent to asking for the entire object)?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's a good idea.

Copy link
Member

@lidel lidel Mar 31, 2023

Choose a reason for hiding this comment

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

I will do it in #245

@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Merging #240 (3867a1a) into main (6f324be) will increase coverage by 0.30%.
The diff coverage is 56.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #240      +/-   ##
==========================================
+ Coverage   47.70%   48.01%   +0.30%     
==========================================
  Files         266      269       +3     
  Lines       32950    32957       +7     
==========================================
+ Hits        15718    15823     +105     
+ Misses      15549    15466      -83     
+ Partials     1683     1668      -15     
Impacted Files Coverage Δ
gateway/blocks_gateway.go 46.33% <0.00%> (+1.78%) ⬆️
gateway/gateway.go 88.52% <ø> (ø)
gateway/handler_unixfs_dir.go 61.96% <50.00%> (-0.93%) ⬇️
gateway/handler_defaults.go 33.76% <61.29%> (+2.25%) ⬆️

... and 19 files with indirect coverage changes

@lidel
Copy link
Member

lidel commented Mar 31, 2023

I've cherry-picked this into #245, will continue there, Incl. Get and GetRange consolidation.

@lidel lidel closed this Mar 31, 2023
@lidel lidel deleted the feat/gateway-fix-range-dir branch March 31, 2023 21:28
lidel added a commit to ipfs-inactive/bifrost-gateway that referenced this pull request Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants