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

fix: pass links to store when adding data #299

Merged
merged 3 commits into from
Oct 11, 2022

Conversation

rklaehn
Copy link
Contributor

@rklaehn rklaehn commented Oct 6, 2022

Note that this may lead to decreased performance, since we are doing more work than before. But the store needs this information.

The way links are parsed can be optimized, see n0-computer/beetle#162 . And I am not quite sure if finding links should not be done on the store side. But that is another discussion to be had.

Fixes n0-computer/beetle#167

@rklaehn rklaehn force-pushed the rklaehn/pass-links-to-store branch from cf1fa7f to 2c2309f Compare October 6, 2022 15:46
@rklaehn rklaehn marked this pull request as ready for review October 6, 2022 16:49
@rklaehn
Copy link
Contributor Author

rklaehn commented Oct 10, 2022

I am now passing through all the links, so at all places where there was a (Cid, Bytes) before there is a (Cid, Bytes, Vec> now. But I don't like it very much. It is a bit verbose and error prone. So will try making a struct and see how that goes...

@rklaehn rklaehn force-pushed the rklaehn/pass-links-to-store branch 2 times, most recently from 3424a56 to 0188ff1 Compare October 10, 2022 13:07
@rklaehn
Copy link
Contributor Author

rklaehn commented Oct 10, 2022

@dignifiedquire had a bit of a fun time merging this with the changes from @ramfox in 6ba6b57 , but I think it turned out OK.

Regarding the tests: I have added validate to block, but I am not quite sure where to put the tests. I can easily validate that the blocks that are produced by file.encode or dir.encode are correct by adding a call to validate in stream_to_resolver, which is used by the roundtrip tests.

But I think what you might want is to actually test that links are properly passed to the store in add_file or add_dir, which would be big enough to do a separate issue. I think there is currently no test at all for unixfs_builder::add_dir or unixfs_builder::add_file. And this would have to be more like an integration test that creates a temp dir etc.

If you want I can add it to this PR, but it will take some time.

Edit: added the former because why not.

@dignifiedquire
Copy link
Contributor

Lets get this in and make a new issue for adding adding tests

dignifiedquire
dignifiedquire previously approved these changes Oct 10, 2022
@rklaehn
Copy link
Contributor Author

rklaehn commented Oct 10, 2022

Made a followup issue for the proposed tests: n0-computer/beetle#152

This had to be merged with some other changes related to the size info.
Introduce a Block struct that has data, hash and links for DRY.
Validate blocks in all roundtrip tests to make sure that the blocks that
are returned from file.encode or dir.encode are internally consistent.
The old code was creating new ids even if we already knew the cid, which
caused some havoc.
Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

nice catch

@rklaehn rklaehn merged commit 9476d9c into n0-computer:main Oct 11, 2022
@rklaehn rklaehn deleted the rklaehn/pass-links-to-store branch October 11, 2022 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Make sure links get persisted in the database
2 participants