feat: support --raw-leaves #219

achingbrain · 2018-07-17T09:43:57Z

Goes some way towards fixing ipfs/js-ipfs#1432 - will need follow up PRs for js-ipfs-mfs and js-ipfs itself (🔜).

There are three ways of importing a file we need to support and each will end up with slightly different DAG structure.

ipfs add will result in a balanced DAG with leaf nodes that are unixfs nodes of type file
ipfs files write results in a trickle DAG with leaf nodes that are unixfs nodes of type raw
ipfs add --raw-leaves and ipfs files write --raw-leaves have the balanced/trickle DAG of above, but the leaf nodes are chunks of file data not wrapped in protobufs.

In all cases above the root node is a unixfs file node with a v0 CID, unless you specify --cid-version=1.

This PR:

Changes the meaning of existing rawLeaves argument. It now means the leaf node is just data - a chunk of the file - this is consistent with go. Previously it was meant a unixfs node with type raw. So far the only code using this is js-ipfs-mfs so changing it shouldn't be too disruptive.
Adds a leafType option which can be file or raw - when --raw-leaves is false, this is what the unixfs leaf type will be.
Uses CIDv1 for raw leaves with the codec raw

parkan · 2018-07-17T15:23:12Z

Great, this makes raw-leaves behavior more consistent between js and go, as well 😄

alanshaw

👍 Looks good so far - I've not had time to run tests and I'd like to do another pass over the code when I get a moment but leaving my feedback anyway 😛

alanshaw · 2018-07-17T15:40:39Z

README.md

@@ -149,7 +149,7 @@ The input's file paths and directory structure will be preserved in the [`dag-pb
 - `onlyHash` (boolean, defaults to false): Only chunk and hash - do not write to disk
 - `hashAlg` (string): multihash hashing algorithm to use
 - `cidVersion` (integer, default 0): the CID version to use when storing the data (storage keys are based on the CID, _including_ it's version)
- `rawLeafNodes` (boolean, defaults to false): When a file would span multiple DAGNodes, if this is true the leaf nodes will be marked as `raw` `unixfs` nodes
+- `rawLeaves` (boolean, defaults to false): When a file would span multiple DAGNodes, if this is true the leaf nodes will not be wrapped in `UnixFS` protobufs and will instead contain the raw file bytes


Document leafType here?

alanshaw · 2018-07-17T15:43:45Z

src/builder/builder.js

+        const file = new UnixFS(options.leafType, buffer)
+
+        DAGNode.create(file.marshal(), [], options.hashAlg, (err, node) => {
+          let cid = new CID(0, 'dag-pb', node.multihash)


Handle err before this? node would be null right?

alanshaw · 2018-07-17T15:49:30Z

src/builder/builder.js

+
+          if (options.cidVersion === 1) {
+            cid = cid.toV1()
+          }


Super minor suggestion - you can probably short circuit this and just pass options.cidVersion || 0 to the constructor above. toV1 will create a new instance and validate it.

Same same for reduce.js

alanshaw

I'm happy to approve after the one requested change. This LGTM and tests all pass.

alanshaw · 2018-07-18T15:36:22Z

src/builder/reduce.js

      }
-    ], (err, node) => {
+    ], (err, {node, cid}) => {


You can't destructure like this in a callback.

If err, then the result will be undefined/null and will try to destructure the properties anyway and throw.

Goes some way towards fixing ipfs/js-ipfs#1432 - will need follow up PRs for js-ipfs-mfs and js-ipfs itself (🔜). There are three ways of importing a file we need to support and each will end up with slightly different DAG structure. ipfs add will result in a balanced DAG with leaf nodes that are unixfs nodes of type file ipfs files write results in a trickle DAG with leaf nodes that are unixfs nodes of type raw ipfs add --raw-leaves and ipfs files write --raw-leaves have the balanced/trickle DAG of above, but the leaf nodes are chunks of file data not wrapped in protobufs. In all cases above the root node is a unixfs file node with a v0 CID, unless you specify --cid-version=1. This PR: Changes meaning of existing rawLeaves argument. Now means the leaf node is just data - a chunk of the file, previously it was meant a unixfs node with type raw. So far the only code using this is js-ipfs-mfs so changing it shouldn't be too disruptive. Adds a leafType option which can be file or raw - when --raw-leaves is false, this is what the unixfs leaf type will be. Uses CIDv1 for raw leaves with the codec raw

daviddias

Great work @achingbrain ❤️! Didn't see anything alarming but I must confess that this module is starting to get a bit unwieldy.

It seems it is time to break the multiple importers into their own packages (and document them nicely).

daviddias · 2018-07-19T06:43:26Z

src/builder/reduce.js

-          DAGNode.create(fileNode.marshal(), [], options.hashAlg, (err, node) => {
-            cb(err, { DAGNode: node, fileNode: fileNode })
+          DAGNode.create(fileNode.marshal(), [], options.hashAlg, (error, node) => {
+            cb(error, { DAGNode: node, fileNode: fileNode })


Nitpick, why s/err/error?

Personal preference really - I think short variable names make the code less expressive.

achingbrain · 2018-07-19T07:28:28Z

this module is starting to get a bit unwieldy

Agreed, it's long overdue a refactor - there's a lot of conditional logic that could be eliminated.

It seems it is time to break the multiple importers into their own packages (and document them nicely).

Possibly - if this module was decomposed into lots of smaller modules, could you see them being consumed independently?

It would certainly introduce a bunch of administrative overhead - maybe we could trial it as a monorepo to alleviate some of that burden?

ghost assigned achingbrain Jul 17, 2018

ghost added the status/in-progress In progress label Jul 17, 2018

achingbrain force-pushed the support-raw-leaf-nodes branch from d1356df to 438f3b4 Compare July 17, 2018 12:42

achingbrain requested review from alanshaw and daviddias July 17, 2018 13:41

achingbrain mentioned this pull request Jul 17, 2018

feat: support --raw-leaves ipfs-inactive/js-ipfs-mfs#5

Merged

alanshaw reviewed Jul 17, 2018

View reviewed changes

achingbrain force-pushed the support-raw-leaf-nodes branch from 438f3b4 to 9105db1 Compare July 17, 2018 16:23

alanshaw suggested changes Jul 18, 2018

View reviewed changes

achingbrain force-pushed the support-raw-leaf-nodes branch from 9105db1 to 7a29d83 Compare July 18, 2018 16:39

daviddias approved these changes Jul 19, 2018

View reviewed changes

alanshaw approved these changes Jul 19, 2018

View reviewed changes

achingbrain merged commit 8d0b025 into master Jul 19, 2018

ghost removed the status/in-progress In progress label Jul 19, 2018

achingbrain deleted the support-raw-leaf-nodes branch July 19, 2018 09:14

achingbrain mentioned this pull request Sep 24, 2018

2018 Q4 OKRs Planning ipfs/js-ipfs#1566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support --raw-leaves #219

feat: support --raw-leaves #219

achingbrain commented Jul 17, 2018 •

edited

Loading

parkan commented Jul 17, 2018

alanshaw left a comment

alanshaw Jul 17, 2018

achingbrain Jul 17, 2018

alanshaw Jul 17, 2018

alanshaw Jul 17, 2018

alanshaw left a comment

alanshaw Jul 18, 2018

daviddias left a comment

daviddias Jul 19, 2018

achingbrain Jul 19, 2018

achingbrain commented Jul 19, 2018

feat: support --raw-leaves #219

feat: support --raw-leaves #219

Conversation

achingbrain commented Jul 17, 2018 • edited Loading

parkan commented Jul 17, 2018

alanshaw left a comment

Choose a reason for hiding this comment

alanshaw Jul 17, 2018

Choose a reason for hiding this comment

achingbrain Jul 17, 2018

Choose a reason for hiding this comment

alanshaw Jul 17, 2018

Choose a reason for hiding this comment

alanshaw Jul 17, 2018

Choose a reason for hiding this comment

alanshaw left a comment

Choose a reason for hiding this comment

alanshaw Jul 18, 2018

Choose a reason for hiding this comment

daviddias left a comment

Choose a reason for hiding this comment

daviddias Jul 19, 2018

Choose a reason for hiding this comment

achingbrain Jul 19, 2018

Choose a reason for hiding this comment

achingbrain commented Jul 19, 2018

achingbrain commented Jul 17, 2018 •

edited

Loading