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

include a root in car responses #355

Merged
merged 1 commit into from
Jul 13, 2023
Merged

include a root in car responses #355

merged 1 commit into from
Jul 13, 2023

Conversation

willscott
Copy link
Contributor

@willscott willscott commented Jul 13, 2023

fix #354

This is a somewhat naive fix, but i don't think it's totally unreasonable.

If you ask for /ipfs/<cid>/path/to/file
the root of the car you get back will, with this change, be <cid>.

I think the complexity that led to the current structure was a desire to have the root cid be file, but for validation and since the response car has blocks that are parents of file, it doesn't seem "incorrect" to say that the root of the structure in the car is <cid>.

ref: ipfs/specs#402 (comment)

@willscott
Copy link
Contributor Author

(complementary PR is ipld/go-car#461 which makes parsing more lenient)

Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2023

Codecov Report

Merging #355 (5d20703) into main (d5e275d) will increase coverage by 0.12%.
The diff coverage is 80.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #355      +/-   ##
==========================================
+ Coverage   76.21%   76.34%   +0.12%     
==========================================
  Files          85       85              
  Lines        6394     6395       +1     
==========================================
+ Hits         4873     4882       +9     
+ Misses       1249     1243       -6     
+ Partials      272      270       -2     
Impacted Files Coverage Δ
cmd/lassie/fetch.go 32.71% <0.00%> (ø)
pkg/server/http/ipfs.go 65.96% <100.00%> (+1.57%) ⬆️
pkg/storage/deferredstoragecar.go 71.76% <100.00%> (+3.90%) ⬆️

... and 1 file with indirect coverage changes

@willscott willscott merged commit 5686890 into main Jul 13, 2023
@willscott willscott deleted the feat/include-root branch July 13, 2023 23:51
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.

Write a root on car responses
3 participants