-
Notifications
You must be signed in to change notification settings - Fork 17
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
Support incoming and outgoing duplicates #249
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
==========================================
+ Coverage 69.87% 70.75% +0.88%
==========================================
Files 68 71 +3
Lines 5918 6162 +244
==========================================
+ Hits 4135 4360 +225
- Misses 1541 1559 +18
- Partials 242 243 +1
|
I've now refactored some of the work to make it more flexible and added integration tests. I feel generally happy with where things are at though I am still working. I'm marking ready for review. It may be that we need to seperate out CAR verifier into two classes based on the new lower level interface, but also there is going to be some duped code if we do that. |
d4461cb
to
3dc1323
Compare
for _, acceptType := range acceptTypes { | ||
typeParts := strings.Split(acceptType, ";") | ||
if typeParts[0] == "*/*" || typeParts[0] == "application/*" || typeParts[0] == "application/vnd.ipld.car" { | ||
validAccept = true | ||
break | ||
if typeParts[0] == "application/vnd.ipld.car" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wew, this really does need refactoring out to a separate "accept" header decoder func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually went ahead and refactored all of the ipfs handler file, but then decided this should be a seperate PR cause... lots more changes.
case "order": | ||
// for now, dfs traversal satisfies all known orders | ||
default: | ||
// ignore others |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could also reject on unknown params, but it's not as clear case as with novel orders, this could be something to bring up in 412 - do we want to proactively reject unknown options so upgrade paths are clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, for now, not known
"github.com/filecoin-project/lassie/pkg/types" | ||
"github.com/filecoin-project/lassie/pkg/verifiedcar" | ||
"github.com/ipfs/go-cid" | ||
"github.com/ipfs/go-libipfs/blocks" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:shakes-fist-at-sky:
I think this is sane. I've only done an initial pass, I'm going to have to reflect on the approach but I think my biggest concern is around increasing the complexity of the already complex CAR layering: temporary, w/ preload support, deferred, streaming. I don't know if there's an easy way to reduce that complexity so maybe just more documentation around it would help. |
e527ef4
to
9d20893
Compare
Co-authored-by: Rod Vagg <rod@vagg.org>
Co-authored-by: Rod Vagg <rod@vagg.org>
Co-authored-by: Rod Vagg <rod@vagg.org>
Co-authored-by: Rod Vagg <rod@vagg.org>
Co-authored-by: Rod Vagg <rod@vagg.org>
Co-authored-by: Rod Vagg <rod@vagg.org>
Goals
Support github.com/ipfs/specs/pulls/412
Implementation
This adds two primary pieces of functionality:
The key adds are:
There are still some critical race condition issues here relating to io.Pipe, and I need to figure out how to better mitigate them
However, I think this is the baseline architecture.
Also needs further integration tests but wanted to validate the general approach