Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix: allow reading rawLeaves in MFS #4282

Merged
merged 3 commits into from Jan 11, 2023
Merged

Conversation

RangerMauve
Copy link
Contributor

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

This comment was marked as resolved.

@RangerMauve
Copy link
Contributor Author

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

@achingbrain
Copy link
Member

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

@lidel lidel changed the title Allow reading rawLeaves in MFS fix: allow reading rawLeaves in MFS Jan 10, 2023
@RangerMauve
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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

@RangerMauve
Copy link
Contributor Author

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

@achingbrain
Copy link
Member

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.

@achingbrain achingbrain merged commit 0cfcaf6 into ipfs:master Jan 11, 2023
@RangerMauve
Copy link
Contributor Author

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
Copy link
Contributor

tabcat commented Jan 11, 2023

@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
Copy link
Contributor Author

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
Copy link
Member

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 #4252 is merged, likely tomorrow morning.

@RangerMauve
Copy link
Contributor Author

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

@achingbrain
Copy link
Member

achingbrain commented Jan 12, 2023

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants