search

ipfs / js-ipfs

IPFS implementation in JavaScript
https://js.ipfs.tech
Other
7.43k stars 1.25k forks source link

fix: allow reading rawLeaves in MFS #4282

Closed RangerMauve closed 1 year ago

RangerMauve commented 1 year ago

Currently, if you write to an MFS directory with rawLeaves: true, it'll error out when doing a read saying Error: /example-0/example.txt was not a file.

This accounts for that case by handling the raw type the same as a file.

There might be other places this should be looked at but this was an immediate blocker I was dealing with.

This might also be something we should fix in the exporter?

welcome[bot] commented 1 year ago

Thank you for submitting this PR! A maintainer will be here shortly to review it. We are super grateful, but we are also overloaded! Help us by making sure that:

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment. Next steps:

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. We are very grateful for your contribution!

RangerMauve commented 1 year ago

If someone needs a temporary workaround, either apply this patch manually, or avoid using rawLeaves: true with MFS.

achingbrain commented 1 year ago

Thanks for submitting this - could you add a test please, to prevent future regressions.

RangerMauve commented 1 year ago

@achingbrain Yeah sure, which file should I add the test to? I see that files.read is only used in tests/preload.spec.js

Should I create a new file?

achingbrain commented 1 year ago

Awesome, thanks - please add it to the interface suite: https://github.com/ipfs/js-ipfs/blob/master/packages/interface-ipfs-core/src/files/read.js

RangerMauve commented 1 year ago

Pushed a test, also checked that the test was failing before the fix so this should catch future issues.

RangerMauve commented 1 year ago

Yikes, not sure what';s up with the CI errors, but I don't think it's a result of the PR

achingbrain commented 1 year ago

The new test is failing when run against kubo. I've fixed it up by copying the added file to the MFS first, rather than using MFS methods to read the block using the ipfs path.

This is a bug with kubo really, it should be able to handle raw leaves.

RangerMauve commented 1 year ago

TY for the quick turnaround time. 😁

Is this something that will be auto-released or is there somewhere I can track the release schedule?

tabcat commented 1 year ago

@RangerMauve most of the js ipfs/libp2p repos use release-please action which, on push to master, publishes an rc and preps a release pr.

RangerMauve commented 1 year ago

I notice some of the jobs failed, does that mean I need to wait for the next successful release for the publish? https://github.com/ipfs/js-ipfs/actions/runs/3895148505/jobs/6650213839

achingbrain commented 1 year ago

An rc is published with every successful merge to master.

The current HEAD is passing so this is available to try out via npm i ipfs-core@next, or wait for the release which will go out when https://github.com/ipfs/js-ipfs/pull/4252 is merged, likely tomorrow morning.

RangerMauve commented 1 year ago

Perfect thank you. I'll subscribe to that and update my deps accordingly. 🙇

achingbrain commented 1 year ago

This is a bug with kubo really, it should be able to handle raw leaves.

Actually looking back on this, I don't think it's a bug in kubo. The original test used /ipfs/${cid} as the path to read, js-ipfs figures out it's an IPFS path and reads it from the blockstore. Kubo treats it as an MFS path, which doesn't exist, hence the file does not exist error.

Adding the CID to the MFS with a file name and then reading that works because both js-ipfs and kubo can resolve the file name as an MFS path.

The approach Kubo takes, although less flexible, is probably easier to reason about so is probably preferable.